Skip to content

Conversation

robertsipka
Copy link
Contributor

No description provided.

uint32_t dummy; /* dummy member for alignment */
#endif
} mem_pools_chunk_t;
} mem_pool_chunk;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rewrite doesn't seem to be equivalent to the original version. This code defines a struct mem_pool_chunk -- and then also defines a variable named mem_pool_chunk of type struct mem_pool_chunk. This global variable is certainly not what we want.

BTW, why do we need this rewrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. I changed this with another solution.
This modification fixed the following warnings:
150:22: assignment from incompatible pointer type : mem_free_chunk_p = chunk_p->next_p;
173:27: assignment from incompatible pointer type : chunk_to_free_p->next_p = mem_free_chunk_p;
190:39: initialization from incompatible pointer type : mem_pools_chunk_t *const next_p = mem_free_chunk_p->next_p;

{
JERRY_DDLOG ("RegExp is found in cache\n");
return re_cache[*idx];
return (re_compiled_code_t *) re_cache[*idx];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change re_cache to store re_compiled_code_t pointers. This is what is there anyway.

@robertsipka
Copy link
Contributor Author

Thanks for the comments. I've updated the patch.
@zherczeg: At the first solution I didn't choose the safest way. I considered that and eliminated these warnings with adding the missing const modifiers, which I think is much closer to the original purpose. Please take a look at it.

* Node for free chunk list
*/
typedef struct
typedef struct mem_pools_chunk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. However, there are some more recursive structs in the code base, which solve the the issue inconsitently, sometimes even incorrectly. Could you please open an issue or a follow-up PR to have a uniform solution for all recursive structs out there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I would prefer a naming like mem_pools_chunk_forward_t in general

@LaszloLango LaszloLango added the bug Undesired behaviour label Mar 8, 2016
@LaszloLango
Copy link
Contributor

LGTM

1 similar comment
@zherczeg
Copy link
Member

zherczeg commented Mar 8, 2016

LGTM

ECMA_TRY_CATCH (empty,
re_parse_char_class (re_ctx_p->parser_ctx_p,
re_append_char_class,
(re_char_class_callback) re_append_char_class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, it is bad practice to cast function pointers. Instead, change re_append_char_class to match re_char_class_callback's type and cast the uint32_t to ecma_char_t inside the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franc0is, good point. @robertsipka, please change typedef void (*re_char_class_callback) (void *re_ctx_p, uint32_t start, uint32_t end); to typedef void (*re_char_class_callback) (void *re_ctx_p, ecma_char_t start, ecma_char_t end); and remove the cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for the comments. I've updated the patch.
@LaszloLango : This modification will affect some helper functions, and more code refactoring needed. I think we could open a follow-up PR to do this kind of conversions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Passing argument 1 of ‘strncmp’ from incompatible pointer type.
Assignments from incompatible pointer types.
Passing argument or initialization discards ‘const’ qualifier from pointer target type.

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka [email protected]
@robertsipka
Copy link
Contributor Author

Rebased with master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Undesired behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants