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.

Sign up for GitHub

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

Add "gsl" to #includes #1184

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Add "gsl" to #includes #1184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/gsl/algorithm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@beinhaerter beinhaerter Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://learn.microsoft.com/en-us/cpp/build/reference/i-additional-include-directories?view=msvc-170

The compiler searches directories in the following order:

  1. 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.
  2. 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.
  3. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carsonRadtke thoughts on replacing quotes with angle brackets? I presume this might make @beinhaerter more comfortable with the change.

Copy link
Contributor

@beinhaerter beinhaerter Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. GSL-path/gsl/gsl/assert (relative to gsl/span)
  2. GSL-path/gsl/gsl/assert (relative to gsl/algorithm)
  3. PROJ-path/gsl/assert (relative to a.cpp) => this can cause problems!
  4. 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.

Copy link
Collaborator

@carsonRadtke carsonRadtke Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#include "gsl/span" // for dynamic_extent, span

#include <algorithm> // for copy_n
#include <cstddef> // for ptrdiff_t
Expand Down
16 changes: 8 additions & 8 deletions include/gsl/gsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@
#define GSL_GSL_H

// IWYU pragma: begin_exports
#include "algorithm" // copy
#include "assert" // Ensures/Expects
#include "byte" // byte
#include "pointers" // owner, not_null
#include "span" // span
#include "zstring" // zstring
#include "util" // finally()/narrow_cast()...
#include "gsl/algorithm" // copy
#include "gsl/assert" // Ensures/Expects
#include "gsl/byte" // byte
#include "gsl/pointers" // owner, not_null
#include "gsl/span" // span
#include "gsl/zstring" // zstring
#include "gsl/util" // finally()/narrow_cast()...

#ifdef __cpp_exceptions
#include "narrow" // narrow()
#include "gsl/narrow" // narrow()
#endif
// IWYU pragma: end_exports

Expand Down
4 changes: 2 additions & 2 deletions include/gsl/narrow
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

#ifndef GSL_NARROW_H
#define GSL_NARROW_H
#include "assert" // for GSL_SUPPRESS
#include "util" // for narrow_cast
#include "gsl/assert" // for GSL_SUPPRESS
#include "gsl/util" // for narrow_cast
#include <exception> // for std::exception
namespace gsl
{
Expand Down
2 changes: 1 addition & 1 deletion include/gsl/pointers
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#ifndef GSL_POINTERS_H
#define GSL_POINTERS_H

#include "assert" // for Ensures, Expects
#include "gsl/assert" // for Ensures, Expects

#include <cstddef> // for ptrdiff_t, nullptr_t, size_t
#include <functional> // for less, greater
Expand Down
8 changes: 4 additions & 4 deletions include/gsl/span
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
#ifndef GSL_SPAN_H
#define GSL_SPAN_H

#include "assert" // for Expects
#include "byte" // for byte
#include "span_ext" // for span specialization of gsl::at and other span-related extensions
#include "util" // for narrow_cast
#include "gsl/assert" // for Expects
#include "gsl/byte" // for byte
#include "gsl/span_ext" // for span specialization of gsl::at and other span-related extensions
#include "gsl/util" // for narrow_cast

#include <array> // for array
#include <cstddef> // for ptrdiff_t, size_t, nullptr_t
Expand Down
4 changes: 2 additions & 2 deletions include/gsl/span_ext
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
//
///////////////////////////////////////////////////////////////////////////////

#include "assert" // GSL_KERNEL_MODE
#include "util" // for narrow_cast, narrow
#include "gsl/assert" // GSL_KERNEL_MODE
#include "gsl/util" // for narrow_cast, narrow

#include <cstddef> // for ptrdiff_t, size_t
#include <utility>
Expand Down
2 changes: 1 addition & 1 deletion include/gsl/string_span
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#pragma once
#pragma message( \
"This header will soon be removed. Use <gsl/zstring> instead of <gsl/string_span>")
#include "zstring"
#include "gsl/zstring"
2 changes: 1 addition & 1 deletion include/gsl/util
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#ifndef GSL_UTIL_H
#define GSL_UTIL_H

#include "assert" // for Expects
#include "gsl/assert" // for Expects

#include <array>
#include <cstddef> // for ptrdiff_t, size_t
Expand Down
2 changes: 1 addition & 1 deletion include/gsl/zstring
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#ifndef GSL_ZSTRING_H
#define GSL_ZSTRING_H

#include "span_ext" // for dynamic_extent
#include "gsl/span_ext" // for dynamic_extent

#include <cstddef> // for size_t, nullptr_t

Expand Down
Loading