Add "gsl" to #includes #1184
Add "gsl" to #includes #1184
Conversation
Office is seeing build breaks due to `#include "span"` including C++20 span instead of gsl/span. Most likely we want all headers includes qualified with "gsl/" to avoid similar issues.
| @@ -17,8 +17,8 @@ | |||
| #ifndef GSL_ALGORITHM_H | |||
| #define GSL_ALGORITHM_H | |||
|
|
|||
| #include "assert" // for Expects | |||
| #include "span" // for dynamic_extent, span | |||
| #include "gsl/assert" // for Expects | |||
There was a problem hiding this comment.
I know, include search order is implementation defined. But from what I see from current compilers:
Includes with "" are searched in the current path first. In the current path here there is no subfolder "gsl/".
If at all then it should be <gsl/...>, but <> is for system/external includes, not for include from the same project.
In my opinion this is a change that makes things worse, not better. I bet it's a problem with the Office build setttings and it should be fixed there, not here.
There was a problem hiding this comment.
https://learn.microsoft.com/en-us/cpp/build/reference/i-additional-include-directories?view=msvc-170
The compiler searches directories in the following order:
- If the #include directive is specified using double-quote form, it first searches local directories. The search begins in the same directory as the file that contains the #include directive. If it fails to find the file, it searches next in the directories of the currently opened include files, in the reverse order in which they were opened. The search begins in the directory of the parent include file and continues upward through the directories of any grandparent include files.
- If the #include directive is specified in angle-bracket form, or if the local directory search has failed, it searches directories specified by using the /I option, in the order they're specified on the command line.
- Directories specified in the INCLUDE environment variable.
So the Office code needs to #include <gsl/...>, but the GSL itself should #include "...". If GSL itself does #include "gsl/..." then this fails and what is found then depends on the parent include files and if they have a gsl subfolder. If that all fails step three triggers. That is a lot of fragile searching. On the other side #include "..." just finds the file aside the GSL file that is doing the include.
There was a problem hiding this comment.
What we were observing is that gsl/algorithm@21 and gsl/gsl@25 where including the C++20 std::span headers. Our code does use the fully qualified include.
There was a problem hiding this comment.
I am inclined to revert this PR. I fear we may have blocked customers from installing gsl under a different name (for example, they might #include <ms-gsl/gsl>).
What we were observing is that gsl/algorithm@21 and gsl/gsl@25 where including the C++20 std::span headers. Our code does use the fully qualified include.
I don't want reverting this PR to mean that Office needs to apply a patch every time y'all consume GSL. @tiagomacarios Can you link to a pipeline where the wrong span was included? Maybe there are some clues there that will help us understand why.
There was a problem hiding this comment.
@beinhaerter Office uses header units. When header units are enabled the compiler prefers a header map (not in the docs you linked). It is unclear if this is by design or not. We are following up with the compiler team.
Anyway, path should be fully qualified. See boost or any other large opensource project for examples.
There was a problem hiding this comment.
@carsonRadtke thoughts on replacing quotes with angle brackets? I presume this might make @beinhaerter more comfortable with the change.
There was a problem hiding this comment.
The key is the last sentence - it will look for "gsl/gsl/..." and then since it won't find it, it will look for "gsl/../gsl/..." and will then find it.
My reading of the docs is different. The docs don't say that the parent directory of the current include is searched. It says that the directory of the parent include is searched. Given
- PROJ-path/a.cpp including <gsl/algorithm>
- GSL-path/gsl/algorithm including "span"
- GSL-path/gsl/span including "gsl/assert"
gsl/assert will be searched as:
- GSL-path/gsl/gsl/assert (relative to gsl/span)
- GSL-path/gsl/gsl/assert (relative to gsl/algorithm)
- PROJ-path/gsl/assert (relative to a.cpp) => this can cause problems!
- GSL-path/gsl/assert (relative to path in INCLUDE environment variable)
Office is using /headerUnit:angle span=<...> which will lookup import "span" using the same rules #include .
So shouldn't this work?
/headerUnit:angle span=<...>
/headerUnit:angle gsl/span=<...>
#include <span>
#include <gsl/span>
or
import <span>;
import <gsl/span>;
How is the import done in Office for span? And what is provided for the headerUnit of gsl/span?
Do you have a minimal compilable example where the wrong behaviour can be observed? Otherwise I will try to reproduce it myself.
There was a problem hiding this comment.
Sorry for the delay - this fell off my radar last week.
My reading of the docs is different.
You're right, I misinterpreted that.
So shouldn't this work?
Yes, this would work; Office is in the process of moving to header units for library dependencies, but the process takes time.
@beinhaerter If you believe this new behavior is bad enough to warrant a change, we can do one of the following:
<gsl/...>(as was discussed earlier)"./..."(as was suggested by the modules FE dev in an internal chat)
There was a problem hiding this comment.
My two cents:
As the GSL is there to support the guidelines it should itself follow the guidelines. That means GSL should #include "..." and not #include <...>.
The MS Office team has a solution that works with GSL doing #include "span", they only need to implement it. Until then they could modify their local copy of the GSL.
So I would revert the change and let the Office team solve the problem on their side (either in the build system or in their GSL copy). I would not see a reason to solve their problem in the open source GSL project.
If you want to wait with reverting the change until the Office team is ready, then an #include "./span" looks appealing because it follows the guidelines (even if the "./" looks a bit strange) and it works for the Office team. As of today I revert the change in my local copy of GSL. If you want to go the "./span" way I would just use that original upstream code.
There was a problem hiding this comment.
If you want to wait with reverting the change until the Office team is ready, then an #include "./span" looks appealing because it follows the guidelines (even if the "./" looks a bit strange) and it works for the Office team.
I will create a PR that reverts this PR and takes this approach. @tiagomacarios Any objections?
Office is seeing build breaks due to
#include "span"including C++20 span instead of gsl/span. Most likely we want all headers includes qualified with "gsl/" to avoid similar issues.See https://office.visualstudio.com/Office/_git/Office/pullrequest/3657798