Closed giuliopaci closed 8 years ago
I'm sorry, please wait a few days. I'm in a business trip and have several deadlines. I will check and consider it on next Wednesday.
Ok, perfect.
Since I have free time, I checked your comment and another pull request. I am going to accept your pull request after sending this message.
I think that your idea sounds good. On concern is that the user must be set the path. If possible, I wish that your request does not force the user to set the path. (For example, the test script uses the direct path. The user can use the program without setting the path. On the other hand, the user can also set the path as necessary.)
How is this idea?
I hope you understand that it is not possible not to force users to set a path, either implicitly or explicitly.
The current include directives in World work by-chance, as no standard defines how relative paths must be handled and they are still searched in the headers path.
The standard defines #include <>
and #include ""
as global and local respectively, but is up to the preprocessor to decide what this means. You can have a look at gcc definitions (https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html) to get an idea of a possible compiler decision about global/local definition.
You may also have a look at http://stackoverflow.com/a/597438/3021900 to understand possible bad implications about using relative paths in include directives.
Finally, as you aim at following Google C++ Style Guide, I point you to https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes where they go even one step further, by saying that
All of a project's header files should be listed as descendants of the project's source directory
so that all the users will always be forced to explicitly set the proper path.
I am going to prepare a pull request that will move headers from src
to src/world
. When compiling files in test, -Isrc
option should be used. I will change the include directives in *.cpp/*.h
files to be relative to the source directory (in accordance to Google C++ Style Guide and to what I consider best practice), with the exception of files in src/world
, in accordance to your desire to limit the requirements for users to explicitly set an header path at compile time.
If you want to remove the exception in src/world
files (in accordance to Google C++ Style Guide), I will be happy to perform the required changes.
Files in test refers to both world
headers and local headers. This is not to be intended as an exception as the files in src
and the files in test
are, logically, two different projects.
I think that moving header is good and reasonable. To put it simply, my concern is the case that the user uses Visual Studio (VS). To compile the program, I think the user must set the absolute path in the project file.
On concern is that the user must be set the path.
This means that the user must be set the absolute path according to their development environments. I am going to search whether we can set the relative path or not. If it is possible, I modify the project file of VS and will accept the pull request.
I suspected your issue was about Visual Studio. As far as I remember (last time I did real development with Visual Studio was 8 years ago) it is perfectly possible to use relative paths in linker options. Maybe the only issue will be the fact that you probably will have to directly manually edit the options, rather than using file selectors.
For Visual Studio users, the user defined include path can be set by the Additional Include Directories property. https://msdn.microsoft.com/en-us/library/73f9s62w.aspx
Thank you very much for your suggestion. I confirmed that we can set the relative paths as the Additional Include Directories. After sending this message, I will accept the pull request. And then I will revise the VS project file and add a comment in the path setting.
That is great.
If you agree, I will also complete the changes, so that also the headers in world/
directory will refer to each other using the world/
prefix, as recommended in Google C++ Style Guide.
Ok, I confirmed the changes. I accept your pull request.
Currently the World source code contains include lines that uses headers names as if they where file names. While this approach is working, it must be considered that headers are searched in a path and that a better approach to avoid name collisions is to move headers in a "world" directory, so that the directory name will act as a prefix. When the correct headers path is defined, this will let people change the include directives from:
include "./../src/dio.h"
to
include "world/dio.h" or #include <world/dio.h>
It should be also nice to provide a general header so that it is possible, for a user of the library, to simply specify
include <world/world.h>
If you agree with this change, I will prepare a pull request for it.