Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

clang-format does not recognize full path header, e.g. <.../MainModule.h>, as corresponding header when sorting #26633

Open Quuxplusone opened 8 years ago

Quuxplusone commented 8 years ago
Bugzilla Link PR26634
Status CONFIRMED
Importance P normal
Reported by Tobias Markmann (tm@ayena.de)
Reported on 2016-02-16 07:54:48 -0800
Last modified on 2020-06-03 06:48:23 -0700
Version unspecified
Hardware All All
CC ababos@bloomberg.net, ddomenichelli@drdanz.it, djasper@google.com, klimek@google.com, llvm-bugs@lists.llvm.org, marejde@gmail.com, mydeveloperday@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
A source file located under Name/Folder/MainModule.cpp and the corresponding
header file at Name/Folder/MainModule.h. The source file starts with including
the corresponding header like

#include <Name/Folder/MainModule.h>
#include <cstdio>
#include <Name/Folder/SomeOtherHeader.h>
.

When running "clang-format -i Name/Folder/MainModule.cpp" with sorting in the
.clang-format configuration, it does not put the corresponding header, i.e.
<Name/Folder/MainModule.h>, first but sorts it all alphabetically. Changing it
to a local include like #include "MainModule.h" it is sorted first. Why does it
not recognize the corresponding header file when it's prefixed with a subpath?
Quuxplusone commented 5 years ago
This is because clang::tooling::IncludeCategoryManager::isMainHeader ignores
any header that's not included with quotes:
https://github.com/llvm-mirror/clang/blob/c104c07a624f422cb229a8cc94c9433f144029dc/lib/Tooling/Inclusions/HeaderIncludes.cpp#L178

This should be a very simple 2 line fix, just remove that quotes check.
Quuxplusone commented 4 years ago
But isn't the <> in #include <XXX> supposed to be reserved for system headers?

if we removed that check and imagine you were developing a class call list.cxx

#include "list.h"

#include <vector>
#include <list>

which would be the main header?  #include "list.h" or #include <list>
Quuxplusone commented 4 years ago
1. The meaning of <> vs. "" is implementation defined:
https://en.cppreference.com/w/cpp/preprocessor/include. In practice a lot of
C++ shops use <> for includes coming from the include path, and "" for relative
paths.
2. Since the main module is typically included first, an acceptable heuristic
in case of a conflict would be to pick the first suitable one.
3. We could also define a filter for standard headers to further avoid that
conflict.
4. How else would you suggest resolving the issue Tobias outlined? Assuming
changing the include style is not an option.
Quuxplusone commented 4 years ago
I still feel reading cppreference is that the intent is that <> are for those
supported by -isystem and " is for those by -i certainly I know I don't use "
for just relative includes, but thats probably projecting my own opinion ;-)

However I think as you say the issue is easily avoided by allow both, I don't
think we could do that without making it an option in the .clang-format file or
previously sorted includes could potentially start moving around

If someone would like to propose a name for a suitable option (In my view the
hardest part of this issue), I'l be happy to propose a potential change

Here's my input, but I'm sure someone could think of a better name

AllowSytemHeadersAsMainHeader: true?
AllowChevronHeadersAsMainHeader: true?
Quuxplusone commented 4 years ago

Either works... Although Chevron might be a bit more obscure for non-native speakers. An alternative suggestion could be to name it after the current behavior, which would be:

ExcludeSytemHeadersAsMainHeader

Quuxplusone commented 4 years ago

I like ExcludeSytemHeadersAsMainHeader much better