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

Regularized #include under include/mruby/*.h #3032

Closed
wants to merge 1 commit into from

Conversation

pbosetti
Copy link
Contributor

@pbosetti pbosetti commented Nov 23, 2015

According to C Preprocessor syntax (see the manual), the #include "file" syntax

searches for a file named file first in the directory containing the current file, then in the quote directories and then the same directories used for <file>.

The original call, e.g. #include "mruby/common.h" from within the mruby folder, requires to have -Ipath/to/mruby/include/mruby in compilation (which is not always the case).

Conversely, the current PR is always working (i.e. regardless compilation switches), because mruby/array.h is always able to find #include "common.h" since it is in its same directory.

https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

According to C Preprocessor syntax, the #include "file" syntax first searches in
the directory containing the current file, then in the quote directories
and then the same directories used for <file>.
The original call, e.g. #include "mruby/common.h" from within the mruby folder,
requires to have -Ipath/to/mruby/include/mruby in compilation (which is not
always the case).
@matz
Copy link
Member

@matz matz commented Nov 27, 2015

I'd rather refer headers by <mruby/string.h>. I think that's what I wanted.

@pbosetti
Copy link
Contributor Author

@pbosetti pbosetti commented Nov 27, 2015

@matz bear with me and please forgive me for insisting, but your answer looks contrary to the intended usage for #include as explained by the GCC preprocessor manual.
May I ask why "that's what you wanted"?

Including the file in the very local directory is what you want. Conversely, using angle brackets means "go search for mruby/something.h under the current research path. Please, think about it and spend some time explaining why do you think I am wrong.

IMHO, using angle brackets rather than using double quotes with local path has a possible issue when the user has another version of mruby installed in the system path, because in this case the headers under /usr/local/include/mruby(possibly different from current revision) are included rather those in the build folder.

I still think that using e.g. #include "common.h" is a better solution, and I would really like why you have a different opinion.

Thanks,
Paolo

@matz
Copy link
Member

@matz matz commented Nov 27, 2015

"common.h" may be an exception but other headers (e.g. "mruby/irep.h") are supposed to be included from users code in the form of #include "mruby/irep.h". I want to keep same API throughout all code.

@pbosetti
Copy link
Contributor Author

@pbosetti pbosetti commented Nov 27, 2015

@matz: Let's make this example. Suppose that the file include/irep.h has

#include "common.h"
#include "compile.h"

at lines 10 and 11. When he needs to include irep.h in his own code, the user needs to #include <mruby/irep.h> (as you like to have as API), and to add -I./local/path/to/mruby/include to the compiler flags. It simply works, and the file irep.h, in turn, is including the files common.h and compile.h that are within ./local/path/to/mruby/include/mruby (exactly what is intended to do).

Conversely, with the current code at lines 10 and 11 of include/irep.h:

#include <mruby/common.h>
#include <mruby/compile.h>

it still works, but there is the risk that, if the user has something like /usr/local/include/mruby/common.h installed in his system the C preprocessor includes that file rather than the intended one under ./local/path/to/mruby/include/mruby. If the installed version is different from the local one, the result can be unexpected and even difficult to debug.

Again, using the local include is not changing the API in any way, it is just making the code stronger w.r.t. different possible build conditions.

@matz
Copy link
Member

@matz matz commented Nov 28, 2015

For internal headers, e.g. common.h (in include directory) and node.h (in src directory), you are right and I fixed it. But other headers, mruby/string.h for example, is a part of API, and a user should specify their place by -I anyway. So including them by <> should not have any problem.

If you have a real error case (not theoretical one), I'd happy to be corrected.

@pbosetti
Copy link
Contributor Author

@pbosetti pbosetti commented Nov 30, 2015

@matz I have made a real case here: https://gist.github.com/pbosetti/152379270f53d2386643
It is a very simplified example of our current state, where we are providing at the user with a library (which embeds mruby) and a bunch of public headers. For clarity and isolation, mruby headers are actually under a subdirectory of the main headers folder.

The example script shows how using local include commands in mruby headers (when they refer each other and by using relative paths to the current file) the user only has to add the main header folder to the headers path (with -I) and then in his own code refer to the desired headers with relative to that folder. The provided example emulates this.

Conversely, with the current code in HEAD, this solution would not work and the user has to needlessly add another -I flag with no additional benefit.

I am aware that there are other ways for solving the same problem, but using local includes in mruby headers has the following advantages:

  1. it lets the user to freely choose the header directory structure most suitable for his application
  2. it only requires one -I flag on compilation
  3. most importantly, the change is not affecting in any way pre-existing code, because local include command (#include "local file.h") searches starting from the current file directory both on GCC, Clang, and VisualStudio compilers (actually, preprocessors).

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