Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix building with LINE_INFO #5048

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aksdfauytv
Copy link

@aksdfauytv aksdfauytv commented Mar 9, 2023

config.h has to be included first

JerryScript-DCO-1.0-Signed-off-by: Maciej Musiał xt1@o2.pl

@akosthekiss
Copy link
Member

Please do explain why it would be necessary to hoist the include out of the JERRY_LINE_INFO guard. In which configuration does it cause any problems?

@aksdfauytv
Copy link
Author

When you modify config.h so that JERRY_LINE_INFO is defined as 1 by default, the project no longer compiles.

@akosthekiss
Copy link
Member

I am pretty sure that the root cause for that is not in this file. It may be true that the ecma-globals.h header is needed somewhere where it is not included, but moving the include in jerry-core/ecma/base/ecma-line-info.h out of the guard is certainly a quick'n'dirty fix only.

If there was an issue report for the problem (with properly filled in details) then it were easier to reproduce the build fail and better see which files do need to be fixed actually.

@aksdfauytv
Copy link
Author

aksdfauytv commented Mar 9, 2023

It is generally considered good practice to make your includes independent of who is including them. It is not the case with ecma-line-info.h:
ecma-line-info.h references JERRY_LINE_INFO before including "ecma-globals.h", which defines it. This makes whoever includes ecma-line-info.h responsible for including "ecma-globals.h" first. However, "emac-line-info.c", "js-parser-line-info-create.c" don't do that, and therefore they don't compile (all definitions from ecma-line-info.h are missing).

The project only compiles as is, if you define JERRY_LINE_INFO as a global compiler switch with command line argument -D.

@akosthekiss
Copy link
Member

Well, right. So, the problem is that config.h is not included before the guard. And the other problem is that the project rarely includes config.h directly, but often lazily relies on ecma-globals.h to include config.h transitively.

I am not totally sure which way to go. A) Include config.h before the guard and ecma-globals.h within the guard, which is the "proper" way, but not really used consistently throughout the code base. B) Or just be sloppy like the rest of the code and include ecma-globals.h before the guard even if we only need config.h.

@zherczeg ? others?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants