Skip to content

Insert headers in global module fragment #151624

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

Merged
HighCommander4 merged 6 commits into from
Aug 3, 2025
Merged

Insert headers in global module fragment #151624

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7e79ed
[libtooling] Insert headers in global module fragment
MythreyaK Aug 1, 2025
973ace6
code review
MythreyaK Aug 2, 2025
e044938
code review
MythreyaK Aug 3, 2025
dc275e9
update comment for `getMinHeaderInsertionOffset`
MythreyaK Aug 3, 2025
908392a
Add comment from code review
MythreyaK Aug 3, 2025
e56d504
Merge branch 'main' into mythreyak/module-header-insertion
HighCommander4 Aug 3, 2025
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
22 changes: 21 additions & 1 deletion clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ void skipComments(Lexer &Lex, Token &Tok) {
return;
}

bool checkAndConsumeModuleDecl(const SourceManager &SM, Lexer &Lex,
Token &Tok) {
bool Matched = Tok.is(tok::raw_identifier) &&
Tok.getRawIdentifier() == "module" &&
!Lex.LexFromRawLexer(Tok) && Tok.is(tok::semi) &&
!Lex.LexFromRawLexer(Tok);
return Matched;
}

// Returns the offset after header guard directives and any comments
// before/after header guards (e.g. #ifndef/#define pair, #pragma once). If no
// header guard is present in the code, this will return the offset after
Expand All @@ -95,7 +104,17 @@ unsigned getOffsetAfterHeaderGuardsAndComments(StringRef FileName,
return std::max(InitialOffset, Consume(SM, Lex, Tok));
});
};
return std::max(

auto ModuleDecl = ConsumeHeaderGuardAndComment(
[](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned {
if (checkAndConsumeModuleDecl(SM, Lex, Tok)) {
skipComments(Lex, Tok);
return SM.getFileOffset(Tok.getLocation());
}
return 0;
});

auto HeaderAndPPOffset = std::max(
// #ifndef/#define
ConsumeHeaderGuardAndComment(
[](const SourceManager &SM, Lexer &Lex, Token Tok) -> unsigned {
Expand All @@ -115,6 +134,7 @@ unsigned getOffsetAfterHeaderGuardsAndComments(StringRef FileName,
return SM.getFileOffset(Tok.getLocation());
return 0;
}));
return std::max(HeaderAndPPOffset, ModuleDecl);
Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Technically, this function is no longer doing what its name says, getOffsetAfterHeaderGuardsAndComments.

Can we rename it to getMinHeaderInsertionOffset, and adjust the comment above it to talk about the module declaration as well?

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 3, 2025

Choose a reason for hiding this comment

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

Thanks, done!

Does the comment look okay?

Copy link

@JVApen JVApen Aug 3, 2025

Choose a reason for hiding this comment

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

If rather say something like:

Determines the area where we want to insert header includes. This will be put (when available):
 - after `#pragma once`
 - in-between header guards (`#ifdef/#define` & `#endif`)
 - after opening global module (`module;`)

Feel free to rephrase or ignore

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This does read better, yeah. Will push after current CI run to prevent a restart.

Edit: done

}

// Check if a sequence of tokens is like
Expand Down
142 changes: 142 additions & 0 deletions clang/unittests/Tooling/HeaderIncludesTest.cpp
View file
Open in desktop
Copy link

@JVApen JVApen Aug 2, 2025

Choose a reason for hiding this comment

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

I like the test cases you wrote. I do think we should add at least one extra where we already had some includes in the source code. Such that we know this works in such case as well. (I don't expect failure based on your code)

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the explanation here!

Should I combine all into the same test case, or are 3 separate ones okay?

I just realized the other tests seem to combine multiple checks into one gtest test.

Edit: Ah, it seems I was mistaken, kept them as 3 separate tests. Let me know if the naming looks okay!

Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,148 @@ TEST_F(HeaderIncludesTest, CanDeleteAfterCode) {
EXPECT_EQ(Expected, remove(Code, "\"b.h\""));
}

TEST_F(HeaderIncludesTest, InsertInGlobalModuleFragment) {
// Ensure header insertions go only in the global module fragment
std::string Code = R"cpp(// comments

// more comments

module;
export module foo;

int main() {
std::vector<int> ints {};
})cpp";
std::string Expected = R"cpp(// comments

// more comments

module;
#include <vector>
export module foo;

int main() {
std::vector<int> ints {};
})cpp";

auto InsertedCode = insert(Code, "<vector>");
EXPECT_EQ(Expected, insert(Code, "<vector>"));
}

TEST_F(HeaderIncludesTest, InsertInGlobalModuleFragmentWithPP) {
// Ensure header insertions go only in the global module fragment
std::string Code = R"cpp(// comments

// more comments

// some more comments

module;

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// comment

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// more comment

int main() {
std::vector<int> ints {};
})cpp";
std::string Expected = R"cpp(// comments

// more comments

// some more comments

module;

#include <vector>
#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// comment

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// more comment

int main() {
std::vector<int> ints {};
})cpp";

EXPECT_EQ(Expected, insert(Code, "<vector>"));
}

TEST_F(HeaderIncludesTest, InsertInGlobalModuleFragmentWithPPIncludes) {
// Ensure header insertions go only in the global module fragment
std::string Code = R"cpp(// comments

// more comments

// some more comments

module;

#include "header.h"

#include <string>

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// comment

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// more comment

int main() {
std::vector<int> ints {};
})cpp";
std::string Expected = R"cpp(// comments

// more comments

// some more comments

module;

#include "header.h"

#include <string>
#include <vector>

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// comment

#ifndef MACRO_NAME
#define MACRO_NAME
#endif

// more comment

int main() {
std::vector<int> ints {};
})cpp";

EXPECT_EQ(Expected, insert(Code, "<vector>"));
}

} // namespace
} // namespace tooling
} // namespace clang