search

jerryscript-project / jerryscript

Ultra-lightweight JavaScript engine for the Internet of Things.
https://jerryscript.net
Apache License 2.0
6.93k stars 671 forks source link

fix building with LINE_INFO #5048

Open aksdfauytv opened 1 year ago

aksdfauytv commented 1 year ago

config.h has to be included first

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

akosthekiss commented 1 year ago

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 commented 1 year ago

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

akosthekiss commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?