[clang] [clang] Processing real directories added as virtual ones (PR #91645)
https://github.com/benlangmuir approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/91645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/90925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)
https://github.com/benlangmuir approved this pull request. LGTM if we rename the checkHeaderSearchOptions function. https://github.com/llvm/llvm-project/pull/90925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)
@@ -833,32 +833,33 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions( /// against the header search options in an existing preprocessor. /// /// \param Diags If non-null, produce diagnostics for any mismatches incurred. -static bool checkHeaderSearchOptions(const HeaderSearchOptions , +static bool checkHeaderSearchOptions(llvm::vfs::FileSystem , benlangmuir wrote: I realize you're not changing the _behaviour_ here, but now that we don't even pass in a `HeaderSearchOptions` we should probably rename this. Maybe `checkModuleCachePath`? https://github.com/llvm/llvm-project/pull/90925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)
@@ -839,17 +839,15 @@ static bool checkHeaderSearchOptions(const
HeaderSearchOptions ,
DiagnosticsEngine *Diags,
const LangOptions ,
const PreprocessorOptions ) {
- if (LangOpts.Modules) {
-if (SpecificModuleCachePath != ExistingModuleCachePath &&
-!PPOpts.AllowPCHWithDifferentModulesCachePath) {
- if (Diags)
-Diags->Report(diag::err_pch_modulecache_mismatch)
- << SpecificModuleCachePath << ExistingModuleCachePath;
- return true;
-}
- }
-
- return false;
+ if (!LangOpts.Modules || PPOpts.AllowPCHWithDifferentModulesCachePath ||
+ SpecificModuleCachePath == ExistingModuleCachePath ||
+ llvm::sys::fs::equivalent(SpecificModuleCachePath,
benlangmuir wrote:
That's fine, it will fail when it tries to read anything from the cache.
https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [modules] Accept equivalent module caches from different symlink (PR #90925)
@@ -839,17 +839,15 @@ static bool checkHeaderSearchOptions(const
HeaderSearchOptions ,
DiagnosticsEngine *Diags,
const LangOptions ,
const PreprocessorOptions ) {
- if (LangOpts.Modules) {
-if (SpecificModuleCachePath != ExistingModuleCachePath &&
-!PPOpts.AllowPCHWithDifferentModulesCachePath) {
- if (Diags)
-Diags->Report(diag::err_pch_modulecache_mismatch)
- << SpecificModuleCachePath << ExistingModuleCachePath;
- return true;
-}
- }
-
- return false;
+ if (!LangOpts.Modules || PPOpts.AllowPCHWithDifferentModulesCachePath ||
+ SpecificModuleCachePath == ExistingModuleCachePath ||
+ llvm::sys::fs::equivalent(SpecificModuleCachePath,
benlangmuir wrote:
This should go through the `VirtualFileSystem` to be consistent with the code
that reads module files. The callers of this API should both have easy access
to a VFS to pass in: for `SimplePCHValidator` it's
`FileMgr.getVirtualFileSystem()` and for `PCHValidator` it's
`Reader.getFileManager().getVirtualFileSystem()`.
You could also add a helper method to VirtualFileSystem for `equivalent` that
does `status(Path1).equivalent(status(Path2))` (plus error handling).
https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Allow including module maps to be non-affecting (PR #89992)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/89992 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Process include files changes with -fmodules-validate-input-files-content and -fno-pch-timestamp options (PR #90319)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/90319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Process include files changes with -fmodules-validate-input-files-content and -fno-pch-timestamp options (PR #90319)
@@ -0,0 +1,34 @@ +// UNSUPPORTED: system-windows +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: cd %t +// +// RUN: cp a1.h a.h +// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only -fmodules-cache-path=%t test1.cpp +// RUN: cp a2.h a.h +// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only -fmodules-cache-path=%t test2.cpp benlangmuir wrote: The cc1 version is `-fvalidate-ast-input-files-content`. https://github.com/llvm/llvm-project/pull/90319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Process include files changes with -fmodules-validate-input-files-content and -fno-pch-timestamp options (PR #90319)
@@ -0,0 +1,34 @@ +// UNSUPPORTED: system-windows +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: cd %t +// +// RUN: cp a1.h a.h +// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only -fmodules-cache-path=%t test1.cpp +// RUN: cp a2.h a.h +// RUN: %clang -fmodules -fmodules-validate-input-files-content -Xclang -fno-pch-timestamp -fimplicit-modules -fmodule-map-file=module.modulemap -fsyntax-only -fmodules-cache-path=%t test2.cpp benlangmuir wrote: Does this test need to use the clang driver for some reason, or can it be changed to use `%clang_cc1`, which is generally preferred for tests? https://github.com/llvm/llvm-project/pull/90319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Allow including module maps to be non-affecting (PR #89992)
@@ -249,9 +245,27 @@ GetAffectingModuleMaps(const Preprocessor , Module
*RootModule) {
for (const auto : HS.findResolvedModulesForHeader(*File))
if (const Module *M = KH.getModule())
-CollectIncludingMapsFromAncestors(M);
+CollectModuleMapsForHierarchy(M);
}
+ // FIXME: This algorithm is not correct for module map hierarchies where
benlangmuir wrote:
> Now, we will only walk the module hierarchy and only mark the defining
> MSub2.modulemap and M.modulemap as affecting, effectively creating a "hole"
> in the tree of module map file includes by omitting MSub1.modulemap.
Does this cause a failure when it tries to include MSub1.modulemap? In some
sense this scenario could be "fine" since you have all the modulemaps that
contributed to the module definition.
> However, cases like the following were broken before and will remain broken
> with this patch:
Would being lazier about the module definitions solve this?
I'm not clear what the path to actually fixing these looks like
https://github.com/llvm/llvm-project/pull/89992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Allow including module maps to be non-affecting (PR #89992)
@@ -249,9 +245,27 @@ GetAffectingModuleMaps(const Preprocessor , Module
*RootModule) {
for (const auto : HS.findResolvedModulesForHeader(*File))
if (const Module *M = KH.getModule())
-CollectIncludingMapsFromAncestors(M);
+CollectModuleMapsForHierarchy(M);
}
+ // FIXME: This algorithm is not correct for module map hierarchies where
benlangmuir wrote:
Was this working before, or always broken?
https://github.com/llvm/llvm-project/pull/89992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Support single-file mode for all formats (PR #88764)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/88764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Support single-file mode for all formats (PR #88764)
@@ -1,23 +1,10 @@
// RUN: rm -rf %t
// RUN: split-file %s %t
-//--- missing_tu.json.in
-[{
- "directory": "DIR",
- "command": "clang -fsyntax-only DIR/missing_tu.c",
- "file": "DIR/missing_tu.c"
-}]
-//--- missing_header.json.in
-[{
- "directory": "DIR",
- "command": "clang -fsyntax-only DIR/missing_header.c",
- "file": "DIR/missing_header.c"
-}]
//--- missing_header.c
#include "missing.h"
-// RUN: sed -e "s|DIR|%/t|g" %t/missing_tu.json.in > %t/missing_tu.json
-// RUN: not clang-scan-deps -compilation-database %t/missing_tu.json
2>%t/missing_tu.errs
+// RUN: not clang-scan-deps -- "clang" %t/missing_tu.c 2>%t/missing_tu.errs
benlangmuir wrote:
> Since the scanner is intended to be run on actual compilation jobs, I think
> it's even desirable removing stuff like "-fsyntax-only".
Changing -fsyntax-only to -c would make sense then, but it's weird to me to
have linker steps in scanning unless that's specifically what you're testing.
Anyway, since this one fails before it gets there I guess it doesn't matter,
but I wouldn't want to see tests defaulting to linking.
https://github.com/llvm/llvm-project/pull/88764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Support single-file mode for all formats (PR #88764)
@@ -1,23 +1,10 @@
// RUN: rm -rf %t
// RUN: split-file %s %t
-//--- missing_tu.json.in
-[{
- "directory": "DIR",
- "command": "clang -fsyntax-only DIR/missing_tu.c",
- "file": "DIR/missing_tu.c"
-}]
-//--- missing_header.json.in
-[{
- "directory": "DIR",
- "command": "clang -fsyntax-only DIR/missing_header.c",
- "file": "DIR/missing_header.c"
-}]
//--- missing_header.c
#include "missing.h"
-// RUN: sed -e "s|DIR|%/t|g" %t/missing_tu.json.in > %t/missing_tu.json
-// RUN: not clang-scan-deps -compilation-database %t/missing_tu.json
2>%t/missing_tu.errs
+// RUN: not clang-scan-deps -- "clang" %t/missing_tu.c 2>%t/missing_tu.errs
benlangmuir wrote:
what's up with the quoting around "clang"?
Also, why was -fsyntax-only removed?
https://github.com/llvm/llvm-project/pull/88764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Add `-o` flag to specify output path (PR #88767)
@@ -864,8 +873,24 @@ int clang_scan_deps_main(int argc, char **argv, const
llvm::ToolContext &) {
});
SharedStream Errs(llvm::errs());
- // Print out the dependency results to STDOUT by default.
- SharedStream DependencyOS(llvm::outs());
+
+ std::optional FileOS;
+ llvm::raw_ostream = [&]() -> llvm::raw_ostream & {
+if (OutputFileName == "-")
+ return llvm::outs();
+
+if (OutputFileName == "/dev/null")
+ return llvm::nulls();
+
+std::error_code EC;
+FileOS.emplace(OutputFileName, EC);
+if (EC) {
+ llvm::errs() << llvm::errorCodeToError(EC) << '\n';
benlangmuir wrote:
Would be good to include the fact it's failing during open and what the path is
in this error. Filesystem errrors can be hard to understand without any context
https://github.com/llvm/llvm-project/pull/88767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Add `-o` flag to specify output path (PR #88767)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/88767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Add `-o` flag to specify output path (PR #88767)
@@ -864,8 +873,24 @@ int clang_scan_deps_main(int argc, char **argv, const
llvm::ToolContext &) {
});
SharedStream Errs(llvm::errs());
- // Print out the dependency results to STDOUT by default.
- SharedStream DependencyOS(llvm::outs());
+
+ std::optional FileOS;
+ llvm::raw_ostream = [&]() -> llvm::raw_ostream & {
+if (OutputFileName == "-")
+ return llvm::outs();
benlangmuir wrote:
I don't feel strongly about whether you should change this code or not -- maybe
it's clearer the way it is? -- but `raw_fd_ostream("-", EC)` is going to be the
same as `outs()` anyway.
https://github.com/llvm/llvm-project/pull/88767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Add `-o` flag to specify output path (PR #88767)
https://github.com/benlangmuir approved this pull request. LGTM with a couple minor comments https://github.com/llvm/llvm-project/pull/88767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
https://github.com/benlangmuir approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/68645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][modules] Do not resolve `HeaderFileInfo` externally in `ASTWriter` (PR #87848)
https://github.com/benlangmuir approved this pull request. LGTM (with the `Not to be committed, just here as a demonstration` test removed, of course). https://github.com/llvm/llvm-project/pull/87848 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -194,6 +201,17 @@ class DependencyScanningFilesystemSharedCache {
const CachedFileSystemEntry &
getOrInsertEntryForFilename(StringRef Filename,
const CachedFileSystemEntry );
+
+/// Returns real path associated with the filename or nullptr if none is
benlangmuir wrote:
Nit: "the real path"
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -230,6 +251,26 @@ class DependencyScanningFilesystemLocalCache {
assert(InsertedEntry == && "entry already present");
return *InsertedEntry;
}
+
+ /// Returns real path associated with the filename or nullptr if none is
+ /// found.
+ const CachedRealPath *findRealPathByFilename(StringRef Filename) const {
+assert(llvm::sys::path::is_absolute_gnu(Filename));
+auto It = RealPathCache.find(Filename);
+return It == RealPathCache.end() ? nullptr : It->getValue();
+ }
+
+ /// Associates the given real path with the filename and returns the given
+ /// entry pointer (for convenience).
+ const CachedRealPath &
+ insertRealPathForFilename(StringRef Filename,
+const CachedRealPath ) {
benlangmuir wrote:
Okay, I'm okay with being consistent for now, but I think it's a bad pattern in
general to pass around `const X &` and keep a pointer to it.
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -159,7 +159,37 @@ DependencyScanningFilesystemSharedCache::CacheShard::
getOrInsertEntryForFilename(StringRef Filename,
const CachedFileSystemEntry ) {
std::lock_guard LockGuard(CacheLock);
- return *EntriesByFilename.insert({Filename, }).first->getValue();
+ CacheByFilename[Filename].first =
benlangmuir wrote:
This is changing "get or insert" into "replace".
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -130,11 +130,11 @@ DependencyScanningFilesystemSharedCache::CacheShard::
getOrEmplaceEntryForFilename(StringRef Filename,
llvm::ErrorOr Stat) {
std::lock_guard LockGuard(CacheLock);
- auto Insertion = EntriesByFilename.insert({Filename, nullptr});
- if (Insertion.second)
-Insertion.first->second =
+ const CachedFileSystemEntry *StoredEntry = CacheByFilename[Filename].first;
+ if (!StoredEntry)
+StoredEntry =
benlangmuir wrote:
The original code modified the value in the cache, but doesn't this new code
only put the value in the local variable StoredEntry (i.e. it's not a reference
just a copy)?
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -226,9 +247,28 @@ class DependencyScanningFilesystemLocalCache {
insertEntryForFilename(StringRef Filename,
const CachedFileSystemEntry ) {
assert(llvm::sys::path::is_absolute_gnu(Filename));
-const auto *InsertedEntry = Cache.insert({Filename, }).first->second;
-assert(InsertedEntry == && "entry already present");
-return *InsertedEntry;
+assert(Cache[Filename].first == nullptr && "entry already present");
+Cache[Filename].first =
+return Entry;
benlangmuir wrote:
Nit: I think the original way this was written is slightly better, because in a
no-asserts build if this is ever hit, it returns the entry that's actually in
the map and in an asserts build it is cheaper to only lookup in the map once
instead of twice. I would just keep the old way but insert a pair(,
nullptr)
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][modules] Do not resolve `HeaderFileInfo` externally in `ASTWriter` (PR #87848)
@@ -409,7 +409,7 @@ class SymbolCollector::HeaderFileURICache {
// Framework headers are spelled as , not
// "path/FrameworkName.framework/Headers/Foo.h".
auto = PP->getHeaderSearchInfo();
-if (const auto *HFI = HS.getExistingFileInfo(*FE, /*WantExternal*/ false))
+if (const auto *HFI = HS.getExistingFileInfo(*FE))
benlangmuir wrote:
Could this code be performance sensitive?
https://github.com/llvm/llvm-project/pull/87848
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][modules] Do not resolve `HeaderFileInfo` externally in `ASTWriter` (PR #87848)
@@ -409,7 +409,7 @@ class SymbolCollector::HeaderFileURICache {
// Framework headers are spelled as , not
// "path/FrameworkName.framework/Headers/Foo.h".
auto = PP->getHeaderSearchInfo();
-if (const auto *HFI = HS.getExistingFileInfo(*FE, /*WantExternal*/ false))
+if (const auto *HFI = HS.getExistingFileInfo(*FE))
benlangmuir wrote:
Shouldn't this be `getExistingLocalFileInfo` since it was previously
`/*WantExternal*/ false`?
https://github.com/llvm/llvm-project/pull/87848
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Only compute affecting module maps with implicit search (PR #87849)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/87849 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move state out of `PreprocessorOptions` (2/n) (PR #87099)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/87099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move state out of `PreprocessorOptions` (1/n) (PR #86358)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/86358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move state out of `PreprocessorOptions` (1/n) (PR #86358)
@@ -34,12 +34,17 @@ class InitOnlyAction : public FrontendAction {
/// Preprocessor-based frontend action that also loads PCH files.
class ReadPCHAndPreprocessAction : public FrontendAction {
+ llvm::function_ref OnCI;
benlangmuir wrote:
My concern is that nothing prevents using this frontend action from some other
code where it wouldn't be safe. Maybe we can make it a unique_function?
https://github.com/llvm/llvm-project/pull/86358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move state out of `PreprocessorOptions` (1/n) (PR #86358)
@@ -736,6 +736,19 @@ class Preprocessor {
State ConditionalStackState = Off;
} PreambleConditionalStack;
+ /// Function for getting the dependency preprocessor directives of a file.
+ ///
+ /// These are directives derived from a special form of lexing where the
+ /// source input is scanned for the preprocessor directives that might have
an
+ /// effect on the dependencies for a compilation unit.
+ ///
+ /// Enables a client to cache the directives for a file and provide them
+ /// across multiple compiler invocations.
+ /// FIXME: Allow returning an error.
+ using DependencyDirectivesFn = std::functionhttps://github.com/llvm/llvm-project/pull/86358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Move state out of `PreprocessorOptions` (1/n) (PR #86358)
@@ -34,12 +34,17 @@ class InitOnlyAction : public FrontendAction {
/// Preprocessor-based frontend action that also loads PCH files.
class ReadPCHAndPreprocessAction : public FrontendAction {
+ llvm::function_ref OnCI;
benlangmuir wrote:
What does `OnCI` mean? Is there a clearer name we could choose and/or could we
document the role of this callback?
Also, how do we know function_ref is safe here?
https://github.com/llvm/llvm-project/pull/86358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)
https://github.com/benlangmuir approved this pull request. Thanks for explaining; LGTM https://github.com/llvm/llvm-project/pull/86216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)
@@ -71,6 +71,7 @@
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/module.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/second/module.modulemap",
benlangmuir wrote:
Why did this change?
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Avoid calling expensive `SourceManager::translateFile()` (PR #86216)
@@ -1337,9 +1337,24 @@ static bool compileModule(CompilerInstance
,
// Get or create the module map that we'll use to build this module.
ModuleMap
= ImportingInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+ SourceManager = ImportingInstance.getSourceManager();
bool Result;
- if (OptionalFileEntryRef ModuleMapFile =
- ModMap.getContainingModuleMapFile(Module)) {
+ if (FileID ModuleMapFID = ModMap.getContainingModuleMapFileID(Module);
+ ModuleMapFID.isValid()) {
+// We want to use the top-level module map. If we don't, the compiling
benlangmuir wrote:
What is the connection between this change and the rest of the commit?
https://github.com/llvm/llvm-project/pull/86216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Lazy dependency directives (PR #86347)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/86347 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/84127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)
@@ -301,10 +301,9 @@ bool Module::directlyUses(const Module *Requested) {
if (Requested->isSubModuleOf(Use))
return true;
- // Anyone is allowed to use our builtin stdarg.h and stddef.h and their
- // accompanying modules.
- if (Requested->getTopLevelModuleName() == "_Builtin_stdarg" ||
- Requested->getTopLevelModuleName() == "_Builtin_stddef")
+ // Anyone is allowed to use our builtin stddef.h and its accompanying
modules.
+ if (Requested->fullModuleNameIs({"_Builtin_stddef", "max_align_t"}) ||
+ Requested->fullModuleNameIs({"_Builtin_stddef_wint_t"}))
benlangmuir wrote:
Ah, got it. Thanks for explaining.
https://github.com/llvm/llvm-project/pull/84127
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)
@@ -301,10 +301,9 @@ bool Module::directlyUses(const Module *Requested) {
if (Requested->isSubModuleOf(Use))
return true;
- // Anyone is allowed to use our builtin stdarg.h and stddef.h and their
- // accompanying modules.
- if (Requested->getTopLevelModuleName() == "_Builtin_stdarg" ||
- Requested->getTopLevelModuleName() == "_Builtin_stddef")
+ // Anyone is allowed to use our builtin stddef.h and its accompanying
modules.
+ if (Requested->fullModuleNameIs({"_Builtin_stddef", "max_align_t"}) ||
+ Requested->fullModuleNameIs({"_Builtin_stddef_wint_t"}))
benlangmuir wrote:
Are we not considering the include of `stdarg.h` a use of `_Builtin_stdarg`
here if it doesn't use the modular headers from `_Builtin_stdarg`?
https://github.com/llvm/llvm-project/pull/84127
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)
benlangmuir wrote: >> I'm not excited by the complexity we are moving toward with the builtin >> headers. But I don't have any alternatives. > When -fbuiltin-headers-in-system-modules goes away, things become simpler > than they were since modules were introduced. Even for the case with `-fbuiltin-headers-in-system-modules` things aren't any more complex than they were before https://reviews.llvm.org/D159064 except for the fundamental complexity of having two modes to build in, and hopefully -fbuiltin-headers-in-system-modules goes away. https://github.com/llvm/llvm-project/pull/84127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] giving the __stddef_ headers their own modules can cause redeclaration errors with -fbuiltin-headers-in-system-modules (PR #84127)
@@ -301,10 +301,9 @@ bool Module::directlyUses(const Module *Requested) {
if (Requested->isSubModuleOf(Use))
return true;
- // Anyone is allowed to use our builtin stdarg.h and stddef.h and their
- // accompanying modules.
- if (Requested->getTopLevelModuleName() == "_Builtin_stdarg" ||
- Requested->getTopLevelModuleName() == "_Builtin_stddef")
+ // Anyone is allowed to use our builtin stddef.h and its accompanying
modules.
+ if (Requested->fullModuleNameIs({"_Builtin_stddef", "max_align_t"}) ||
+ Requested->fullModuleNameIs({"_Builtin_stddef_wint_t"}))
benlangmuir wrote:
Why was stdarg removed?
https://github.com/llvm/llvm-project/pull/84127
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [NFC] [C++20] [Modules] [P1689] [Scanner] Don't use thread pool in P1689 per file mode (PR #84285)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/84285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix dependency scanning with -working-directory (PR #84525)
https://github.com/benlangmuir closed https://github.com/llvm/llvm-project/pull/84525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix dependency scanning with -working-directory (PR #84525)
https://github.com/benlangmuir updated
https://github.com/llvm/llvm-project/pull/84525
>From 24e2454b90c2aaabb999e84240d5b2263ff01719 Mon Sep 17 00:00:00 2001
From: Ben Langmuir
Date: Fri, 8 Mar 2024 09:53:42 -0800
Subject: [PATCH] [clang][deps] Fix dependency scanning with -working-directory
Stop overriding -working-directory to CWD during argument parsing, which
should no longer necessary after we set the VFS working directory, and
create a new FileManager after parsing arguments so that
working-directory behaves correctly.
---
.../DependencyScanningWorker.cpp | 14 -
.../ClangScanDeps/working-directory-option.c | 30 +++
2 files changed, 37 insertions(+), 7 deletions(-)
create mode 100644 clang/test/ClangScanDeps/working-directory-option.c
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 2b882f8a5e0793..76f3d950a13b81 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -296,7 +296,7 @@ class DependencyScanningAction : public tooling::ToolAction
{
DisableFree(DisableFree), ModuleName(ModuleName) {}
bool runInvocation(std::shared_ptr Invocation,
- FileManager *FileMgr,
+ FileManager *DriverFileMgr,
std::shared_ptr PCHContainerOps,
DiagnosticConsumer *DiagConsumer) override {
// Make a deep copy of the original Clang invocation.
@@ -342,12 +342,13 @@ class DependencyScanningAction : public
tooling::ToolAction {
ScanInstance.getHeaderSearchOpts().ModulesIncludeVFSUsage =
any(OptimizeArgs & ScanningOptimizations::VFS);
-ScanInstance.setFileManager(FileMgr);
// Support for virtual file system overlays.
-FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
+auto FS = createVFSFromCompilerInvocation(
ScanInstance.getInvocation(), ScanInstance.getDiagnostics(),
-FileMgr->getVirtualFileSystemPtr()));
+DriverFileMgr->getVirtualFileSystemPtr());
+// Create a new FileManager to match the invocation's FileSystemOptions.
+auto *FileMgr = ScanInstance.createFileManager(FS);
ScanInstance.createSourceManager(*FileMgr);
// Store the list of prebuilt module files into header search options. This
@@ -624,9 +625,8 @@ bool DependencyScanningWorker::computeDependencies(
ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
auto = ModifiedFS ? ModifiedFS : BaseFS;
- FileSystemOptions FSOpts;
- FSOpts.WorkingDir = WorkingDirectory.str();
- auto FileMgr = llvm::makeIntrusiveRefCnt(FSOpts, FinalFS);
+ auto FileMgr =
+ llvm::makeIntrusiveRefCnt(FileSystemOptions{}, FinalFS);
std::vector FinalCCommandLine(FinalCommandLine.size(),
nullptr);
llvm::transform(FinalCommandLine, FinalCCommandLine.begin(),
diff --git a/clang/test/ClangScanDeps/working-directory-option.c
b/clang/test/ClangScanDeps/working-directory-option.c
new file mode 100644
index 00..d57497d405d30f
--- /dev/null
+++ b/clang/test/ClangScanDeps/working-directory-option.c
@@ -0,0 +1,30 @@
+// Test that -working-directory works even when it differs from the working
+// directory of the filesystem.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/other
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format
experimental-full \
+// RUN: > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK: "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/cwd/t.c"
+// CHECK-NEXT: "[[PREFIX]]/cwd/relative/h1.h"
+// CHECK-NEXT: ]
+// CHECK-NEXT: "input-file": "[[PREFIX]]/cwd/t.c"
+
+//--- cdb.json.template
+[{
+ "directory": "DIR/other",
+ "command": "clang -c t.c -I relative -working-directory DIR/cwd",
+ "file": "DIR/cwd/t.c"
+}]
+
+//--- cwd/relative/h1.h
+
+//--- cwd/t.c
+#include "h1.h"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix dependency scanning with -working-directory (PR #84525)
benlangmuir wrote: I don't think there's a difference we can test for here -- the VFS WD shouldn't be modified after the driver sets it and before the FM is used here, so it would be identical to `-working-directory` during the critical window. But I agree recreating the FileManager is probably a better solution since it defines away this problem. I'll look at doing it that way. https://github.com/llvm/llvm-project/pull/84525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix dependency scanning with -working-directory (PR #84525)
benlangmuir wrote: > I can see a situation where we ask FileManager about the same relative path > before and after setting the parsed FileSystemOptions. The second call would > blindly return the cached result, effectively ignoring -working-directory for > that file. The driver calls `VFS->setCurrentWorkingDirectory` using the value of `-working-directory`. So in general relative paths will be resolved consistently with `-working-directory` if they're seen before configuring the `FileManager`. Now, a fair question is why are we changing both the VFS working directory *and* making paths absolute in the FileManager for `-working-directory`, which seems redundant, but I'm not looking to change that behaviour -- this PR should make scanning behave more consistently with compilation. To be clear this situation with mismatched relative paths would **currently** be broken, because the way we were setting `FSOpts.WorkingDir` was not using the value of `-working-directory` before this change. https://github.com/llvm/llvm-project/pull/84525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix dependency scanning with -working-directory (PR #84525)
https://github.com/benlangmuir created
https://github.com/llvm/llvm-project/pull/84525
Stop overriding -working-directory to CWD during argument parsing, which should
no longer necessary after we set the VFS working directory, and set FSOpts
correctly after parsing arguments so that working-directory behaves correctly.
>From e214b51d85fd203a6ec8be3d5aef6b3aa3d8d741 Mon Sep 17 00:00:00 2001
From: Ben Langmuir
Date: Fri, 8 Mar 2024 09:53:42 -0800
Subject: [PATCH] [clang][deps] Fix dependency scanning with -working-directory
Stop overriding -working-directory to CWD during argument parsing, which
should no longer necessary after we set the VFS working directory, and
set FSOpts correctly after parsing arguments so that working-directory
behaves correctly.
---
.../DependencyScanningWorker.cpp | 8 +++--
.../ClangScanDeps/working-directory-option.c | 30 +++
2 files changed, 35 insertions(+), 3 deletions(-)
create mode 100644 clang/test/ClangScanDeps/working-directory-option.c
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 2b882f8a5e0793..f7c302854a2479 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -342,6 +342,9 @@ class DependencyScanningAction : public tooling::ToolAction
{
ScanInstance.getHeaderSearchOpts().ModulesIncludeVFSUsage =
any(OptimizeArgs & ScanningOptimizations::VFS);
+// FileMgr was created before option parsing; set its FileSystemOptions
now.
+FileMgr->getFileSystemOpts() = ScanInstance.getFileSystemOpts();
+
ScanInstance.setFileManager(FileMgr);
// Support for virtual file system overlays.
FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
@@ -624,9 +627,8 @@ bool DependencyScanningWorker::computeDependencies(
ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
auto = ModifiedFS ? ModifiedFS : BaseFS;
- FileSystemOptions FSOpts;
- FSOpts.WorkingDir = WorkingDirectory.str();
- auto FileMgr = llvm::makeIntrusiveRefCnt(FSOpts, FinalFS);
+ auto FileMgr =
+ llvm::makeIntrusiveRefCnt(FileSystemOptions{}, FinalFS);
std::vector FinalCCommandLine(FinalCommandLine.size(),
nullptr);
llvm::transform(FinalCommandLine, FinalCCommandLine.begin(),
diff --git a/clang/test/ClangScanDeps/working-directory-option.c
b/clang/test/ClangScanDeps/working-directory-option.c
new file mode 100644
index 00..d57497d405d30f
--- /dev/null
+++ b/clang/test/ClangScanDeps/working-directory-option.c
@@ -0,0 +1,30 @@
+// Test that -working-directory works even when it differs from the working
+// directory of the filesystem.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/other
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format
experimental-full \
+// RUN: > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK: "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/cwd/t.c"
+// CHECK-NEXT: "[[PREFIX]]/cwd/relative/h1.h"
+// CHECK-NEXT: ]
+// CHECK-NEXT: "input-file": "[[PREFIX]]/cwd/t.c"
+
+//--- cdb.json.template
+[{
+ "directory": "DIR/other",
+ "command": "clang -c t.c -I relative -working-directory DIR/cwd",
+ "file": "DIR/cwd/t.c"
+}]
+
+//--- cwd/relative/h1.h
+
+//--- cwd/t.c
+#include "h1.h"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][ScanDeps] Allow PCHs to have different VFS overlays (PR #82294)
@@ -175,8 +192,19 @@ static void sanitizeDiagOpts(DiagnosticOptions ) {
DiagOpts.ShowCarets = false;
// Don't write out diagnostic file.
DiagOpts.DiagnosticSerializationFile.clear();
- // Don't emit warnings as errors (and all other warnings too).
- DiagOpts.IgnoreWarnings = true;
+ // Don't emit warnings except for scanning specific warnings.
+ // TODO: It would be useful to add a more principled way to ignore all
+ // warnings that come from source code. The issue is that we need to
+ // ignore warnings that could be surpressed by
+ // `#pragma clang diagnostic`, while still allowing some scanning
+ // warnings for things we're not ready to turn into errors yet.
benlangmuir wrote:
> The scanner never sees #pragma clang diagnostic, so there's no issue with
> code that uses that to turn warnings on.
Ah sorry, I forgot we skipped over most pragmas.
> so you're just left with default warnings.
>
> The goal here was to keep driver warnings (which are lost otherwise) and
> allow us to have dedicated scanner warnings. I do think we want more control
> over this, possibly add a scanner bit to diagnostics so we can be explicit
> about which warnings we expect from the scanner, but I think this change is
> fine for now.
This goal makes some sense to me, but I'm not sure that using the default
warnings are a good idea. The default warnings can just as easily cause us to
emit a driver warning that the user was explicitly trying to hide.
https://github.com/llvm/llvm-project/pull/82294
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][ScanDeps] Allow PCHs to have different VFS overlays (PR #82294)
@@ -175,8 +192,19 @@ static void sanitizeDiagOpts(DiagnosticOptions ) {
DiagOpts.ShowCarets = false;
// Don't write out diagnostic file.
DiagOpts.DiagnosticSerializationFile.clear();
- // Don't emit warnings as errors (and all other warnings too).
- DiagOpts.IgnoreWarnings = true;
+ // Don't emit warnings except for scanning specific warnings.
+ // TODO: It would be useful to add a more principled way to ignore all
+ // warnings that come from source code. The issue is that we need to
+ // ignore warnings that could be surpressed by
+ // `#pragma clang diagnostic`, while still allowing some scanning
+ // warnings for things we're not ready to turn into errors yet.
benlangmuir wrote:
I'm concerned about removing `IngoreWarnings = true`. There are lots of
default warnings we may not want. Also this would allow `#pragma clang
diagnostic` to **promote** arbitrary diagnostics to errors.
https://github.com/llvm/llvm-project/pull/82294
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/81061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
@@ -127,8 +127,12 @@ class PPCallbacks {
/// \param RelativePath The path relative to SearchPath, at which the include
/// file was found. This is equal to FileName except for framework includes.
///
- /// \param Imported The module, whenever an inclusion directive was
- /// automatically turned into a module import or null otherwise.
+ /// \param SuggestedModule The module, whenever an inclusion directive was
+ /// considered to be automatically turned into a module import, or null
benlangmuir wrote:
"was considered to be" most commonly it means you are describing the result of
a consideration -- "Clang is considered to be a good compiler" -- rather than
simply saying that something was considered but without reflecting the final
judgment. It's ambiguous here.
Suggestion: "The module suggested for this header, if any. See \p
ModuleImported for whether this include was translated into a module imported"
https://github.com/llvm/llvm-project/pull/81061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
https://github.com/benlangmuir deleted https://github.com/llvm/llvm-project/pull/81061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
@@ -127,8 +127,12 @@ class PPCallbacks {
/// \param RelativePath The path relative to SearchPath, at which the include
/// file was found. This is equal to FileName except for framework includes.
///
- /// \param Imported The module, whenever an inclusion directive was
- /// automatically turned into a module import or null otherwise.
+ /// \param SuggestedModule The module, whenever an inclusion directive was
+ /// considered to be automatically turned into a module import, or null
benlangmuir wrote:
"considered to be" is ambiguous:
https://github.com/llvm/llvm-project/pull/81061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
https://github.com/benlangmuir commented: It would be good to update your commit message to talk about the benefits of this change: the callback now provides more information about the included header and models the associated module accurately regardless of whether it is translated to an import. https://github.com/llvm/llvm-project/pull/81061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
@@ -127,8 +127,12 @@ class PPCallbacks {
/// \param RelativePath The path relative to SearchPath, at which the include
/// file was found. This is equal to FileName except for framework includes.
///
- /// \param Imported The module, whenever an inclusion directive was
- /// automatically turned into a module import or null otherwise.
+ /// \param SuggestedModule The module, whenever an inclusion directive was
+ /// considered to be automatically turned into a module import, or null
benlangmuir wrote:
I think this comment should be updated since it's no longer always a "module
import".
https://github.com/llvm/llvm-project/pull/81061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
@@ -2265,7 +2265,6 @@ Preprocessor::ImportAction
Preprocessor::HandleHeaderIncludeOrImport(
UsableHeaderUnit = true;
else if (!IsImportDecl) {
// This is a Header Unit that we do not include-translate
- SuggestedModule = ModuleMap::KnownHeader();
SM = nullptr;
benlangmuir wrote:
Maybe we should rename `SM` to reflect the difference in behaviour?
https://github.com/llvm/llvm-project/pull/81061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][lex] Always pass suggested module to `InclusionDirective()` callback (PR #81061)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/81061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang] NFC: Remove `{File, Directory}Entry::getName()` (PR #74910)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/74910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang] NFC: Remove `OptionalFileEntryRefDegradesToFileEntryPtr` (PR #74899)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/74899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang] [clang] NFC: Remove `OptionalDirectoryEntryRefDegradesToDirectoryEntryPtr` (PR #74900)
https://github.com/benlangmuir approved this pull request. Nice simplification https://github.com/llvm/llvm-project/pull/74900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] NFCI: Make `ModuleFile::File` non-optional (PR #74892)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/74892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] NFCI: Make `ModuleFile::File` non-optional (PR #74892)
@@ -441,22 +434,19 @@ void
ModuleManager::visit(llvm::function_ref Visitor,
bool ModuleManager::lookupModuleFile(StringRef FileName, off_t ExpectedSize,
time_t ExpectedModTime,
OptionalFileEntryRef ) {
- File = std::nullopt;
- if (FileName == "-")
+ if (FileName == "-") {
+File = expectedToOptional(FileMgr.getSTDIN());
return false;
+ }
// Open the file immediately to ensure there is no race between stat'ing and
// opening the file.
- OptionalFileEntryRef FileOrErr =
- expectedToOptional(FileMgr.getFileRef(FileName, /*OpenFile=*/true,
-/*CacheFailure=*/false));
- if (!FileOrErr)
-return false;
-
- File = *FileOrErr;
+ File = expectedToOptional(FileMgr.getFileRef(FileName, /*OpenFile=*/true,
benlangmuir wrote:
Yeah, I would recommend that.
https://github.com/llvm/llvm-project/pull/74892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] NFCI: Make `ModuleFile::File` non-optional (PR #74892)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/74892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang] NFC: Deprecate `FileEntry::getName()` (PR #68157)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/68157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang] NFC: Deprecate `FileEntry::getName()` (PR #68157)
@@ -157,6 +157,25 @@ #define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]] #endif +// clang-format off +#if defined(__clang__) || defined(__GNUC__) +#define LLVM_IGNORE_DEPRECATIONS_OF_DECLARATIONS_BEGIN \ benlangmuir wrote: I'm not sure how often the general form of this is useful - I don't actually see many places in the code that disable diagnostics for both MSVC and clang/gcc. So I would stick with the version that is specific to deprecations. Maybe you should more closely match the name of `_LIBCPP_SUPPRESS_DEPRECATED_PUSH` -- e.g. `LLVM_SUPPRESS_DEPRECATED_PUSH` and `..._POP`. https://github.com/llvm/llvm-project/pull/68157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][DependencyScanner] Include the working directory in the context hash (PR #73719)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/73719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][DependencyScanner] Remove unused -ivfsoverlay files (PR #73734)
@@ -498,11 +518,18 @@ class NamedNodeOrError {
} // namespace detail
/// An in-memory file system.
-class InMemoryFileSystem : public FileSystem {
+class InMemoryFileSystem : public RTTIExtends {
std::unique_ptr Root;
std::string WorkingDirectory;
bool UseNormalizedPaths = true;
+public:
+ static const char ID;
+ using GetFileContentsCallback =
benlangmuir wrote:
Is this callback type used?
https://github.com/llvm/llvm-project/pull/73734
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][DependencyScanner] Remove unused -ivfsoverlay files (PR #73734)
https://github.com/benlangmuir commented: It's odd to me that tracking is enabled by default. I would have expected tracking be off by default and enabled explicitly for scanning. Similarly, in the modulemap case it could save-and-restore rather than enable the tracking if it was previously off. https://github.com/llvm/llvm-project/pull/73734 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][DependencyScanner] Remove unused -ivfsoverlay files (PR #73734)
@@ -4997,6 +4997,19 @@ ASTReader::ASTReadResult
ASTReader::readUnhashedControlBlockImpl(
F->SearchPathUsage[I] = true;
break;
}
+case VFS_USAGE: {
+ if (!F)
+break;
+ unsigned Count = Record[0];
+ const char *Byte = Blob.data();
+ F->VFSUsage = llvm::BitVector(Count, false);
+ for (unsigned I = 0; I < Count; ++Byte)
+for (unsigned Bit = 0; Bit < 8 && I < Count; ++Bit, ++I)
+ if (*Byte & (1 << Bit))
+F->VFSUsage[I] = true;
benlangmuir wrote:
Probably worth hoisting this read into its own function since we seem to be
accumulating bit vectors.
https://github.com/llvm/llvm-project/pull/73734
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][DependencyScanner] Remove unused -ivfsoverlay files (PR #73734)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/73734 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
benlangmuir wrote:
You could try using `clang -cc1 -E -x c-module-map` which calls
`Module::print`. To trigger this code path you can follow the pattern in
`darwin_specific_modulemap_hacks.m`, ie create a module like
```
$ touch Tcl/x1.h
$ touch Tcl/x2.h
$ cat Tcl/module.modulemap
module Tcl {
module Private {
requires excluded
umbrella ""
}
}
$ clang -cc1 -E -x c-module-map Tcl/module.modulemap -fmodule-name=Tcl
# 1 "Tcl/module.modulemap"
module Tcl {
module Private {
textual header "" { size 0 mtime 1701196669 }
textual header "" { size 0 mtime 1701196741 }
textual header "" { size 75 mtime 1701196655 }
}
}
```
However I think the printing would need to be fixed to print the right header
name instead of "" first.
https://github.com/llvm/llvm-project/pull/73323
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][DependencyScanner] Remove all warning flags when suppressing warnings (PR #71612)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/71612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Skip writing `DIAG_PRAGMA_MAPPINGS` record (PR #70874)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/70874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Revert "Reland [clang] Canonicalize system headers in dependency file when -canonical-prefixes" (PR #71697)
https://github.com/benlangmuir approved this pull request. Thanks! https://github.com/llvm/llvm-project/pull/71697 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] [clang][DepScan] Make OptimizeArgs a bit mask enum and enable by default (PR #71588)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/71588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Avoid modules diagnostics for `__has_include()` (PR #71450)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/71450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Don't prevent translation of FW_Private includes when explicitly building FW (PR #70714)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/70714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/69975 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-scan-deps] [P1689] Keep consistent behavior for make dependencies with clang (PR #69551)
https://github.com/benlangmuir approved this pull request. https://github.com/llvm/llvm-project/pull/69551 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-scan-deps] [P1689] Keep consistent behavior for make dependencies with clang (PR #69551)
@@ -666,13 +666,19 @@ static StringRef
makeAbsoluteAndPreferred(CompilerInstance , StringRef Path,
}
void ModuleDepCollector::addFileDep(StringRef Path) {
- llvm::SmallString<256> Storage;
- Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
+ // Within P1689 format, we don't want all the paths to be absolute path
+ // since it may violate the tranditional make style dependencies info.
+ if (!IsStdModuleP1689Format) {
+llvm::SmallString<256> Storage;
+Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
+ }
FileDeps.push_back(std::string(Path));
benlangmuir wrote:
This is a use-after-free of `Storage` since `Path` will point to that buffer if
the string is modified. Same for the other case below.
https://github.com/llvm/llvm-project/pull/69551
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-scan-deps] [P1689] Keep consistent behavior for make dependencies with clang (PR #69551)
benlangmuir wrote: Thanks for the ping, I had missed your question > How do you think about the idea to add a flag to the MDC about whether or not > calling makeAbsoluteAndPreferred? SGTM; this seems like a good compromise since we can't easily extract this into the consumer. https://github.com/llvm/llvm-project/pull/69551 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -168,6 +170,12 @@ class DependencyScanningFilesystemSharedCache {
/// The backing storage for cached contents.
llvm::SpecificBumpPtrAllocator ContentsStorage;
+/// Map from filenames to cached real paths.
+llvm::StringMap RealPathsByFilename;
benlangmuir wrote:
I would lean towards combining them unless there's a drawback I'm not seeing -
the keys are fairly large here so it's nice to share storage for that. I would
maybe feel differently for a DenseMap of small keys :shrug:
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Cache `VFS::getRealPath()` (PR #68645)
@@ -230,6 +251,26 @@ class DependencyScanningFilesystemLocalCache {
assert(InsertedEntry == && "entry already present");
return *InsertedEntry;
}
+
+ /// Returns real path associated with the filename or nullptr if none is
+ /// found.
+ const CachedRealPath *findRealPathByFilename(StringRef Filename) const {
+assert(llvm::sys::path::is_absolute_gnu(Filename));
+auto It = RealPathCache.find(Filename);
+return It == RealPathCache.end() ? nullptr : It->getValue();
+ }
+
+ /// Associates the given real path with the filename and returns the given
+ /// entry pointer (for convenience).
+ const CachedRealPath &
+ insertRealPathForFilename(StringRef Filename,
+const CachedRealPath ) {
benlangmuir wrote:
Since you are storing a pointer to this not the value I would suggest this take
a pointer parameter. Otherwise it's easy to assume it's being copied and would
be safe to pass a stack variable.
https://github.com/llvm/llvm-project/pull/68645
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)
https://github.com/benlangmuir approved this pull request. > Yeah. I did try to fix up all calls to LookupFile to perform module map > lookup, but a bunch of tests started failing (mostly standard C++ modules > tests IIRC), so there's probably more nuance required there. Okay, I do think this is worth fixing long term, but I don't want to block on it. Your change LGTM in the meantime. https://github.com/llvm/llvm-project/pull/70144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] use relative paths for builtin headers during module compilation (PR #68023)
benlangmuir wrote: Thanks for your patience, I should have looked at this sooner. The change to our downstream test looks expected; the `` buffer changed ```diff -#import "/^tc/lib/clang/18/include/stdbool.h" +#import "stdbool.h" ``` But the actual resolution of which header is used is identical. I'm happy with this change; @jansvoboda11 were you looking at anything else or is this okay for @rmaz to land? https://github.com/llvm/llvm-project/pull/68023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)
https://github.com/benlangmuir commented: Can you clarify how this bug occurs in terms of what information about the header is stored that causes us to get the wrong module? It seems bad that passing `nullptr` to `LookupFile` could cause incorrect behaviour and makes me wonder if we need to always trigger module lookup or if there is another way to fix this that would make it safe to not do module lookup here. https://github.com/llvm/llvm-project/pull/70144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)
@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes : MarshallingInfoNegativeFlag>, HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers. " "This flag will be removed in a future Clang release.">; +defm modules_skip_diagnostic_options : BoolFOption<"modules-skip-diagnostic-options", benlangmuir wrote: Seems worth having a test, and a (frontend) option seems like the easiest way to do that. https://github.com/llvm/llvm-project/pull/69975 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)
@@ -1212,9 +1212,12 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor
,
Record.clear();
}
+ const auto = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
// Diagnostic options.
const auto = Context.getDiagnostics();
const DiagnosticOptions = Diags.getDiagnosticOptions();
+ if (!HSOpts.ModulesSkipDiagnosticOptions) {
benlangmuir wrote:
Now that we're doing this upstream we should re-indent this code.
https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)
@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes : MarshallingInfoNegativeFlag>, HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers. " "This flag will be removed in a future Clang release.">; +defm modules_skip_diagnostic_options : BoolFOption<"modules-skip-diagnostic-options", benlangmuir wrote: Should we do the same for HSOpts here so we can add a test for it? https://github.com/llvm/llvm-project/pull/69975 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-scan-deps] [P1689] Keep consistent behavior for make dependencies with clang (PR #69551)
https://github.com/benlangmuir commented: Is the issue with MDC's FileDeps that we are calling `makeAbsoluteAndPreferred` on the paths? Maybe we could instead move that call into `FullDependencyConsumer`. Or are there other issues? The fact we need to add additional `MDC.IsStdModuleP1689Format` checks in this PR to prevent the normal consumer callbacks is not great. This makes MDC aware of implementation details of the specific output format. https://github.com/llvm/llvm-project/pull/69551 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Use file name as requested (PR #68957)
https://github.com/benlangmuir approved this pull request. Thanks https://github.com/llvm/llvm-project/pull/68957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Use file name as requested (PR #68957)
@@ -1384,10 +1384,10 @@ void ASTWriter::WriteControlBlock(Preprocessor ,
ASTContext ,
SmallVector ModMaps(AdditionalModMaps->begin(),
AdditionalModMaps->end());
llvm::sort(ModMaps, [](FileEntryRef A, FileEntryRef B) {
-return A.getName() < B.getName();
+return A.getNameAsRequested() < B.getNameAsRequested();
});
for (FileEntryRef F : ModMaps)
-AddPath(F.getName(), Record);
+AddPath(F.getNameAsRequested(), Record);
benlangmuir wrote:
How does this relate to the header search part of the change?
https://github.com/llvm/llvm-project/pull/68957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Move `SLocEntry` search into `ASTReader` (PR #66966)
https://github.com/benlangmuir approved this pull request. Latest change to shrink memory use LGTM. https://github.com/llvm/llvm-project/pull/66966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Move `SLocEntry` search into `ASTReader` (PR #66966)
@@ -288,10 +288,12 @@ class ModuleFile {
/// for the entry is SLocEntryOffsetsBase + SLocEntryOffsets[i].
uint64_t SLocEntryOffsetsBase = 0;
- /// Offsets for all of the source location entries in the
- /// AST file.
+ /// Stream bit offsets for all of the source location entries in the AST
file.
const uint32_t *SLocEntryOffsets = nullptr;
+ /// SLocEntry offsets that have been loaded from the AST file.
+ std::vector SLocEntryOffsetLoaded;
benlangmuir wrote:
I don't feel that strongly about 10 MB in a large TU; your suggestion seems
reasonable, but could be a follow up.
https://github.com/llvm/llvm-project/pull/66966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Move `SLocEntry` search into `ASTReader` (PR #66966)
https://github.com/benlangmuir approved this pull request. LGTM; you might need a `std::move` in `readSLocOffset` to appease an older compiler. https://github.com/llvm/llvm-project/pull/66966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Move `SLocEntry` search into `ASTReader` (PR #66966)
@@ -1444,6 +1444,74 @@ llvm::Error ASTReader::ReadSourceManagerBlock(ModuleFile
) {
}
}
+llvm::Expected
+ASTReader::readSLocOffset(ModuleFile *F, unsigned Index) {
+ BitstreamCursor = F->SLocEntryCursor;
+ SavedStreamPosition SavedPosition(Cursor);
+ if (llvm::Error Err = Cursor.JumpToBit(F->SLocEntryOffsetsBase +
+ F->SLocEntryOffsets[Index]))
+return Err;
benlangmuir wrote:
I seem to recall that the Error & -> Expected conversion trips a compiler bug
in some of our supported compilers. I think you need `std::move(Err)` when
returning an l-value.
https://github.com/llvm/llvm-project/pull/66966
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Move `SLocEntry` search into `ASTReader` (PR #66966)
https://github.com/benlangmuir edited https://github.com/llvm/llvm-project/pull/66966 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] Remove preloaded SLocEntries from PCM files (PR #66962)
https://github.com/benlangmuir approved this pull request. Thanks for iterating! I find the current implementation much clearer. The only thing I might quibble about is the "child" vs. "parent" terminology you changed: I think it's fairly ambiguous either way, because the node is the "child" from the perspective of a top-down include hierarchy, but it's the "parent" from the perspective of the bottom-up search. You could maybe change it to IncludedFile or something, but I don't feel very strongly about it. Child is no worse than parent so if you prefer child I don't think you need to change it. https://github.com/llvm/llvm-project/pull/66962 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
