[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
AaronBallman wrote: > > > You can certainly construct cases where the different lexing rules in > > > different language modes allow you to detect which language you're in > > > from within the preprocessor ([1](https://eel.is/c++draft/diff.cpp11.lex) > > > [2](https://eel.is/c++draft/diff.cpp14.lex#2) > > > [3](https://eel.is/c++draft/diff.cpp03.lex#1)) or where enabling more > > > language mode flags may reject valid code. It may be good enough for what > > > the scanner is trying to do, but I think it's a stretch to say that it's > > > sound. > > I'll defer this to @Bigcheese, I don't think we've hit any of these yet. > > > +1; it's definitely not sound to go this approach, though it may work well > > enough in practice to be worth the tradeoff. That said, I think the > > behavior of the dependency scanner ignoring the language options used is > > actually kind of problematic, unless I'm misunderstanding something. This > > means command line options that impact dependency scanning may fail (for > > example, the user might enable SYCL or OpenMP on the command line and that > > may have different dependencies, trigraph support may be important for > > things like `??=include`, freestanding vs not impacts which preprocessor > > macros are predefined, etc. How do we handle that sort of stuff, or do we > > just ignore it? > > This layer of the dependency scanner only lexes the source into a list of > tokens. Preprocessing and dependency discovery happen later and do respect > the command-line options. But the list of tokens depends on things like what features are enabled, right? e.g., `-fchar8_t` introduces a new keyword, while `_Atomic` isn't a keyword in OpenCL, and `bool` is only a keyword in C23 and later but is always a keyword in C++, etc. Ah, but in terms of dependency scanning, whether those are keywords or not probably doesn't matter? And because we expose things like `embed` as a keyword in all language modes, not just C23, the language standard doesn't matter either? > Good point on trigraphs. I think that's similar to the digit separator issue: > we can unconditionally enable this feature on this layer of the dependency > scanner and let the build fail later during compilation itself. Seems reasonable to me. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
jansvoboda11 wrote: > > You can certainly construct cases where the different lexing rules in > > different language modes allow you to detect which language you're in from > > within the preprocessor ([1](https://eel.is/c++draft/diff.cpp11.lex) > > [2](https://eel.is/c++draft/diff.cpp14.lex#2) > > [3](https://eel.is/c++draft/diff.cpp03.lex#1)) or where enabling more > > language mode flags may reject valid code. It may be good enough for what > > the scanner is trying to do, but I think it's a stretch to say that it's > > sound. I'll defer this to @Bigcheese, I don't think we've hit any of these yet. > +1; it's definitely not sound to go this approach, though it may work well > enough in practice to be worth the tradeoff. That said, I think the behavior > of the dependency scanner ignoring the language options used is actually kind > of problematic, unless I'm misunderstanding something. This means command > line options that impact dependency scanning may fail (for example, the user > might enable SYCL or OpenMP on the command line and that may have different > dependencies, trigraph support may be important for things like `??=include`, > freestanding vs not impacts which preprocessor macros are predefined, etc. > How do we handle that sort of stuff, or do we just ignore it? This layer of the dependency scanner only lexes the source into a list of tokens. Preprocessing and dependency discovery happen later and do respect the command-line options. Good point on trigraphs. I think that's similar to the digit separator issue: we can unconditionally enable this feature on this layer of the dependency scanner and let the build fail later during compilation itself. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
AaronBallman wrote: > > > > I guess the general question is - is it acceptable to have the Scanner > > > > operating in a language standard different than the passed in language > > > > mode and different than the compiler language standard? > > > > > > > > > I think that is acceptable. It is kinda hacky, but the lexer and > > > preprocessor are largely independent of the language and the standard. > > > When they do depend on those settings, taking the union of the features > > > and letting the compiler trim it down is still a perfectly sound thing to > > > do. > > > > > > You can certainly construct cases where the different lexing rules in > > different language modes allow you to detect which language you're in from > > within the preprocessor ([1](https://eel.is/c++draft/diff.cpp11.lex) > > [2](https://eel.is/c++draft/diff.cpp14.lex#2) > > [3](https://eel.is/c++draft/diff.cpp03.lex#1)) or where enabling more > > language mode flags may reject valid code. It may be good enough for what > > the scanner is trying to do, but I think it's a stretch to say that it's > > sound. > > +1; it's definitely not sound to go this approach, though it may work well > enough in practice to be worth the tradeoff. That said, I think the behavior > of the dependency scanner ignoring the language options used is actually kind > of problematic, unless I'm misunderstanding something. This means command > line options that impact dependency scanning may fail (for example, the user > might enable SYCL or OpenMP on the command line and that may have different > dependencies, trigraph support may be important for things like `??=include`, > freestanding vs not impacts which preprocessor macros are predefined, etc. > How do we handle that sort of stuff, or do we just ignore it? Ping @jansvoboda11 https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
AaronBallman wrote: > > > I guess the general question is - is it acceptable to have the Scanner > > > operating in a language standard different than the passed in language > > > mode and different than the compiler language standard? > > > > > > I think that is acceptable. It is kinda hacky, but the lexer and > > preprocessor are largely independent of the language and the standard. When > > they do depend on those settings, taking the union of the features and > > letting the compiler trim it down is still a perfectly sound thing to do. > > You can certainly construct cases where the different lexing rules in > different language modes allow you to detect which language you're in from > within the preprocessor ([1](https://eel.is/c++draft/diff.cpp11.lex) > [2](https://eel.is/c++draft/diff.cpp14.lex#2) > [3](https://eel.is/c++draft/diff.cpp03.lex#1)) or where enabling more > language mode flags may reject valid code. It may be good enough for what the > scanner is trying to do, but I think it's a stretch to say that it's sound. +1; it's definitely not sound to go this approach, though it may work well enough in practice to be worth the tradeoff. That said, I think the behavior of the dependency scanner ignoring the language options used is actually kind of problematic, unless I'm misunderstanding something. This means command line options that impact dependency scanning may fail (for example, the user might enable SYCL or OpenMP on the command line and that may have different dependencies, trigraph support may be important for things like `??=include`, freestanding vs not impacts which preprocessor macros are predefined, etc. How do we handle that sort of stuff, or do we just ignore it? https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
zygoloid wrote: > > I guess the general question is - is it acceptable to have the Scanner > > operating in a language standard different than the passed in language mode > > and different than the compiler language standard? > > I think that is acceptable. It is kinda hacky, but the lexer and preprocessor > are largely independent of the language and the standard. When they do depend > on those settings, taking the union of the features and letting the compiler > trim it down is still a perfectly sound thing to do. You can certainly construct cases where the different lexing rules in different language modes allow you to detect which language you're in from within the preprocessor ([1](https://eel.is/c++draft/diff.cpp11.lex) [2](https://eel.is/c++draft/diff.cpp14.lex#2) [3](https://eel.is/c++draft/diff.cpp03.lex#1)) or where enabling more language mode flags may reject valid code. It may be good enough for what the scanner is trying to do, but I think it's a stretch to say that it's sound. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: Here is the revert PR: https://github.com/llvm/llvm-project/pull/94488 https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
jansvoboda11 wrote: > > You can have a project that has both C and C++ implementation files that > > end up including the same header files from the C standard library. One can > > be compiled under C11 (without separator support), the other under C++14 > > (with separator support). > > Thanks. I had considered this very lightly, and in my mind, I thought that > this would result in two separate passes of scanning - and so, two separate > scanning services. Maybe I am wrong? Single scanning service will be used across the entire build, across build targets, regardless of language and the standard of the compilation. > Thank you for the additional context and bits of knowledge, super helpful. > I'll put up a separate PR that does the following: > > 1. reverts the changes within this PR > 2. checks if the lexer has `ParsingPreprocessorDirective` property `true` or > `false` and allow for the numeric lexing to happen in that case. > > Let me know if it is preferable for history etc. to have two separate PRs - > one for the revert and one for the lexer to relax the CPP14/C23 constraint > during the preprocessing phase. Yes, splitting this into two PRs would be great. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: > You can have a project that has both C and C++ implementation files that end > up including the same header files from the C standard library. One can be > compiled under C11 (without separator support), the other under C++14 (with > separator support). Thanks. I had considered this very lightly, and in my mind, I thought that this would result in two separate passes of scanning - and so, two separate scanning services. Maybe I am wrong? Thank you for the additional context and bits of knowledge, super helpful. I'll put up a separate PR that does the following: 1. reverts the changes within this PR 2. checks if the lexer has `ParsingPreprocessorDirective` property `true` or `false` and allow for the numeric lexing to happen in that case. Let me know if it is preferable for history etc. to have two separate PRs - one for the revert and one for the lexer to relax the CPP14/C23 constraint during the preprocessing phase. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
jansvoboda11 wrote: > Thanks for the comments @jansvoboda11 . I am new to all these different > moving parts and want to understand better. I have a few questions. > > > If you concurrently scan the same file under two language standards with > > the same scanning service, it becomes non-deterministic which one gets > > cached in the filesystem cache. > > That is true. But until your comment, I did not even know that it is possible > (and supported) to be able to invoke the same scanning service on the same > file under two language options (say c++14 and c++11). How would someone do > that? Asking so I can test this out locally and try to come up with a better > solution. (Also, why would someone do that?) You can have a project that has both C and C++ implementation files that end up including the same header files from the C standard library. One can be compiled under C11 (without separator support), the other under C++14 (with separator support). > > You need to make the language standard a part of the cache key. > > This was kind of one of my concerns that I had called out here: > https://discourse.llvm.org/t/looking-for-help-with-accessing-langopts-from-the-actual-compiler-invocation/79228/3. > Specifically: > > > would it look a bit off to someone if they were to look at the header for > > DependencyScanningWorkerFilesystem and see that the > > ensureDirectiveTokensArePopulated API took a LangOpts specifically > > Given that `LangOpts` is kind of becoming a feature within > `DependencyScanningWorkerFilesystem`'s APIs, I am kind of inclined towards > having `LangOpts` as part of the cache key for disambiguation - but again, I > am very very new to this. Scanning is often the choke point in builds, so any change that slows it down needs to make up for it in correctness. Adding language options (or just the standard) to the cache key could trigger multiple scans for a single file, which would change the performance quite a bit. Since we have an alternative solution that's still correct, I'd prefer that. > > An alternative solution (that I prefer) is to set up the scanner in a way > > that always accepts ' in integer constants. > > Would this be considered "hacky"? Because if we go this way, the Scanner > would technically be operating in a different language mode for integers, > potentially overriding the language mode arg that was passed in during > invocation. I am not opposed to it - just trying to understand the > implications better. We do turn on specific `LangOpts` (like `Objc`) for the > lexer during the Scanning phase as can be seen here - > > https://github.com/llvm/llvm-project/blob/83646590afe222cfdd792514854549077e17b005/clang/lib/Lex/DependencyDirectivesScanner.cpp#L71-L79 > > . > I guess the general question is - is it acceptable to have the Scanner > operating in a language standard different than the passed in language mode > and different than the compiler language standard? I think that is acceptable. It is kinda hacky, but the lexer and preprocessor are largely independent of the language and the standard. When they do depend on those settings, taking the union of the features and letting the compiler trim it down is still a perfectly sound thing to do. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: Thanks for the comments @jansvoboda11 . I am new to all these different moving parts and want to understand better. I have a few questions. > If you concurrently scan the same file under two language standards with the > same scanning service, it becomes non-deterministic which one gets cached in > the filesystem cache. That is true. But until your comment, I did not even know that it is possible (and supported) to be able to invoke the same scanning service on the same file under two language options (say c++14 and c++11). How would someone do that? Asking so I can test this out locally and try to come up with a better solution. (Also, why would someone do that?) > You need to make the language standard a part of the cache key. This was kind of one of my concerns that I had called out here: https://discourse.llvm.org/t/looking-for-help-with-accessing-langopts-from-the-actual-compiler-invocation/79228/3. Specifically: > would it look a bit off to someone if they were to look at the header for > DependencyScanningWorkerFilesystem and see that the > ensureDirectiveTokensArePopulated API took a LangOpts specifically Given that `LangOpts` is kind of becoming a feature within `DependencyScanningWorkerFilesystem`'s APIs, I am kind of inclined towards having `LangOpts` as part of the cache key for disambiguation - but again, I am very very new to this. > An alternative solution (that I prefer) is to set up the scanner in a way > that always accepts ' in integer constants. Would this be considered "hacky"? Because if we go this way, the Scanner would technically be operating in a different language mode for integers, potentially overriding the language mode arg that was passed in during invocation. I am not opposed to it - just trying to understand the implications better. We do turn on specific `LangOpts` (like `Objc`) for the lexer during the Scanning phase as can be seen here - https://github.com/llvm/llvm-project/blob/83646590afe222cfdd792514854549077e17b005/clang/lib/Lex/DependencyDirectivesScanner.cpp#L71-L79. I guess the general question is - is it acceptable to have the Scanner operating in a language standard different than the passed in language mode and different than the compiler language standard? https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
jansvoboda11 wrote: I don't think this is correct. If you concurrently scan the same file under two language standards with the same scanning service, it becomes non-deterministic which one gets cached in the filesystem cache. For subsequent FS queries the cache might return wrong results, ignoring language options. You need to make the language standard a part of the cache key. An alternative solution (that I prefer) is to set up the scanner in a way that always accepts `'` in integer constants. This works around the caching issue and the build can still fail at compile-time where the language standard kicks in as usual. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
github-actions[bot] wrote: @nishithshah2211 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail [here](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of [LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
https://github.com/cor3ntin closed https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: Thanks for the pointers. What would be the process to merge this in? https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
https://github.com/cor3ntin approved this pull request. I think this makes sense. Thanks a lot for the fix https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: Yes, sorry I missed committing that change. Let me know if there is a better way to test out the changes or add any additional changes. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
https://github.com/nishithshah2211 updated
https://github.com/llvm/llvm-project/pull/93753
>From 46a25038abbcf5ab9eebded6813b4bbb71a44925 Mon Sep 17 00:00:00 2001
From: Nishith Shah
Date: Wed, 29 May 2024 12:34:52 -0700
Subject: [PATCH] Pass LangOpts from CompilerInstance to
DependencyScanningWorker
This commit fixes https://github.com/llvm/llvm-project/issues/88896
by passing LangOpts from the CompilerInstance to
DependencyScanningWorker so that the original LangOpts are
preserved/respected. This makes for more accurate parsing/lexing when
certain language versions or features specific to versions are to be used.
---
.../clang/Lex/DependencyDirectivesScanner.h | 3 +-
.../DependencyScanningFilesystem.h| 3 +-
clang/lib/Frontend/FrontendActions.cpp| 4 +-
clang/lib/Lex/DependencyDirectivesScanner.cpp | 22 +++--
.../DependencyScanningFilesystem.cpp | 4 +-
.../DependencyScanningWorker.cpp | 5 +-
.../Lex/DependencyDirectivesScannerTest.cpp | 82 ---
.../Lex/PPDependencyDirectivesTest.cpp| 3 +-
8 files changed, 95 insertions(+), 31 deletions(-)
diff --git a/clang/include/clang/Lex/DependencyDirectivesScanner.h
b/clang/include/clang/Lex/DependencyDirectivesScanner.h
index 0e115906fbfe5..2f8354dec939f 100644
--- a/clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ b/clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -17,6 +17,7 @@
#ifndef LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
#define LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/ArrayRef.h"
@@ -117,7 +118,7 @@ struct Directive {
bool scanSourceForDependencyDirectives(
StringRef Input, SmallVectorImpl
&Tokens,
SmallVectorImpl &Directives,
-DiagnosticsEngine *Diags = nullptr,
+const LangOptions &LangOpts, DiagnosticsEngine *Diags = nullptr,
SourceLocation InputSourceLoc = SourceLocation());
/// Print the previously scanned dependency directives as minimized source
text.
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index f7b4510d7f7be..9dc20065a09a3 100644
---
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -363,7 +363,8 @@ class DependencyScanningWorkerFilesystem
///
/// Returns true if the directive tokens are populated for this file entry,
/// false if not (i.e. this entry is not a file or its scan fails).
- bool ensureDirectiveTokensArePopulated(EntryRef Entry);
+ bool ensureDirectiveTokensArePopulated(EntryRef Entry,
+ const LangOptions &LangOpts);
/// Check whether \p Path exists. By default checks cached result of \c
/// status(), and falls back on FS if unable to do so.
diff --git a/clang/lib/Frontend/FrontendActions.cpp
b/clang/lib/Frontend/FrontendActions.cpp
index 454653a31534c..eddb2ac0c0834 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -1168,8 +1168,8 @@ void
PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
llvm::SmallVector Tokens;
llvm::SmallVector Directives;
if (scanSourceForDependencyDirectives(
- FromFile.getBuffer(), Tokens, Directives, &CI.getDiagnostics(),
- SM.getLocForStartOfFile(SM.getMainFileID( {
+ FromFile.getBuffer(), Tokens, Directives, CI.getLangOpts(),
+ &CI.getDiagnostics(), SM.getLocForStartOfFile(SM.getMainFileID( {
assert(CI.getDiagnostics().hasErrorOccurred() &&
"no errors reported for failure");
diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp
b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index 0971daa1f3666..fda54d314eef6 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -62,14 +62,17 @@ struct DirectiveWithTokens {
struct Scanner {
Scanner(StringRef Input,
SmallVectorImpl &Tokens,
- DiagnosticsEngine *Diags, SourceLocation InputSourceLoc)
+ DiagnosticsEngine *Diags, SourceLocation InputSourceLoc,
+ const LangOptions &LangOpts)
: Input(Input), Tokens(Tokens), Diags(Diags),
-InputSourceLoc(InputSourceLoc), LangOpts(getLangOptsForDepScanning()),
-TheLexer(InputSourceLoc, LangOpts, Input.begin(), Input.begin(),
+InputSourceLoc(InputSourceLoc),
+LangOpts(getLangOptsForDepScanning(LangOpts)),
+TheLexer(InputSourceLoc, this->LangOpts, Input.begin(), Input.begin(),
Input.end()) {}
- static LangOptions getLangOptsForDepScanning() {
-LangOptions LangOpts;
+ static LangOptions
+ getLangOptsForDepScanning(const LangOptions &invocationLangOpts) {
+LangOptions Lang
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
cor3ntin wrote: Can you add a test for https://github.com/llvm/llvm-project/issues/88896 ? https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: @cor3ntin Thanks for reviewing. I updated the PR to get the tests/build passing. Please take a look when you get a chance. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
https://github.com/nishithshah2211 updated
https://github.com/llvm/llvm-project/pull/93753
>From ae79ebec844a6a308bb370184eab892bd74e8fa1 Mon Sep 17 00:00:00 2001
From: Nishith Shah
Date: Wed, 29 May 2024 12:34:52 -0700
Subject: [PATCH] Pass LangOpts from CompilerInstance to
DependencyScanningWorker
This commit fixes https://github.com/llvm/llvm-project/issues/88896
by passing LangOpts from the CompilerInstance to
DependencyScanningWorker so that the original LangOpts are
preserved/respected. This makes for more accurate parsing/lexing when
certain language versions or features specific to versions are to be used.
---
.../clang/Lex/DependencyDirectivesScanner.h | 3 +-
.../DependencyScanningFilesystem.h| 3 +-
clang/lib/Frontend/FrontendActions.cpp| 4 +--
clang/lib/Lex/DependencyDirectivesScanner.cpp | 22 --
.../DependencyScanningFilesystem.cpp | 4 +--
.../DependencyScanningWorker.cpp | 5 ++--
.../Lex/DependencyDirectivesScannerTest.cpp | 30 +--
.../Lex/PPDependencyDirectivesTest.cpp| 3 +-
8 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/clang/include/clang/Lex/DependencyDirectivesScanner.h
b/clang/include/clang/Lex/DependencyDirectivesScanner.h
index 0e115906fbfe5..2f8354dec939f 100644
--- a/clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ b/clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -17,6 +17,7 @@
#ifndef LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
#define LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/ArrayRef.h"
@@ -117,7 +118,7 @@ struct Directive {
bool scanSourceForDependencyDirectives(
StringRef Input, SmallVectorImpl
&Tokens,
SmallVectorImpl &Directives,
-DiagnosticsEngine *Diags = nullptr,
+const LangOptions &LangOpts, DiagnosticsEngine *Diags = nullptr,
SourceLocation InputSourceLoc = SourceLocation());
/// Print the previously scanned dependency directives as minimized source
text.
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index f7b4510d7f7be..9dc20065a09a3 100644
---
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -363,7 +363,8 @@ class DependencyScanningWorkerFilesystem
///
/// Returns true if the directive tokens are populated for this file entry,
/// false if not (i.e. this entry is not a file or its scan fails).
- bool ensureDirectiveTokensArePopulated(EntryRef Entry);
+ bool ensureDirectiveTokensArePopulated(EntryRef Entry,
+ const LangOptions &LangOpts);
/// Check whether \p Path exists. By default checks cached result of \c
/// status(), and falls back on FS if unable to do so.
diff --git a/clang/lib/Frontend/FrontendActions.cpp
b/clang/lib/Frontend/FrontendActions.cpp
index 454653a31534c..eddb2ac0c0834 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -1168,8 +1168,8 @@ void
PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
llvm::SmallVector Tokens;
llvm::SmallVector Directives;
if (scanSourceForDependencyDirectives(
- FromFile.getBuffer(), Tokens, Directives, &CI.getDiagnostics(),
- SM.getLocForStartOfFile(SM.getMainFileID( {
+ FromFile.getBuffer(), Tokens, Directives, CI.getLangOpts(),
+ &CI.getDiagnostics(), SM.getLocForStartOfFile(SM.getMainFileID( {
assert(CI.getDiagnostics().hasErrorOccurred() &&
"no errors reported for failure");
diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp
b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index 0971daa1f3666..fda54d314eef6 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -62,14 +62,17 @@ struct DirectiveWithTokens {
struct Scanner {
Scanner(StringRef Input,
SmallVectorImpl &Tokens,
- DiagnosticsEngine *Diags, SourceLocation InputSourceLoc)
+ DiagnosticsEngine *Diags, SourceLocation InputSourceLoc,
+ const LangOptions &LangOpts)
: Input(Input), Tokens(Tokens), Diags(Diags),
-InputSourceLoc(InputSourceLoc), LangOpts(getLangOptsForDepScanning()),
-TheLexer(InputSourceLoc, LangOpts, Input.begin(), Input.begin(),
+InputSourceLoc(InputSourceLoc),
+LangOpts(getLangOptsForDepScanning(LangOpts)),
+TheLexer(InputSourceLoc, this->LangOpts, Input.begin(), Input.begin(),
Input.end()) {}
- static LangOptions getLangOptsForDepScanning() {
-LangOptions LangOpts;
+ static LangOptions
+ getLangOptsForDepScanning(const LangOptions &invocationLangOpts) {
+Lan
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
cor3ntin wrote: Thanks for working on that. Your change is pretty much what i expected yes, it's great! https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
nishithshah2211 wrote: @cor3ntin @Sirraide I put up a draft PR to get some feedback and see if the changes here make sense or not. Also tagging @pogo59 since I received good feedback/pointers here: https://discourse.llvm.org/t/looking-for-help-with-accessing-langopts-from-the-actual-compiler-invocation/79228. My main concern is mostly around introducing `LangOpts` within `ensureDirectiveTokensArePopulated` API. I'll add new tests and fix existing tests in the next commit on this PR if the source changes seem reasonable. https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Nishith Kumar M Shah (nishithshah2211)
Changes
This commit fixes https://github.com/llvm/llvm-project/issues/88896 by passing
LangOpts from the CompilerInstance to
DependencyScanningWorker so that the original LangOpts are preserved/respected.
This makes for more accurate parsing/lexing when certain language versions or
features specific to versions are to be used.
---
Full diff: https://github.com/llvm/llvm-project/pull/93753.diff
6 Files Affected:
- (modified) clang/include/clang/Lex/DependencyDirectivesScanner.h (+2-1)
- (modified)
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
(+2-1)
- (modified) clang/lib/Frontend/FrontendActions.cpp (+2-2)
- (modified) clang/lib/Lex/DependencyDirectivesScanner.cpp (+12-8)
- (modified)
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp (+2-2)
- (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
(+3-2)
``diff
diff --git a/clang/include/clang/Lex/DependencyDirectivesScanner.h
b/clang/include/clang/Lex/DependencyDirectivesScanner.h
index 0e115906fbfe5..2f8354dec939f 100644
--- a/clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ b/clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -17,6 +17,7 @@
#ifndef LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
#define LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/ArrayRef.h"
@@ -117,7 +118,7 @@ struct Directive {
bool scanSourceForDependencyDirectives(
StringRef Input, SmallVectorImpl
&Tokens,
SmallVectorImpl &Directives,
-DiagnosticsEngine *Diags = nullptr,
+const LangOptions &LangOpts, DiagnosticsEngine *Diags = nullptr,
SourceLocation InputSourceLoc = SourceLocation());
/// Print the previously scanned dependency directives as minimized source
text.
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index f7b4510d7f7be..9dc20065a09a3 100644
---
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -363,7 +363,8 @@ class DependencyScanningWorkerFilesystem
///
/// Returns true if the directive tokens are populated for this file entry,
/// false if not (i.e. this entry is not a file or its scan fails).
- bool ensureDirectiveTokensArePopulated(EntryRef Entry);
+ bool ensureDirectiveTokensArePopulated(EntryRef Entry,
+ const LangOptions &LangOpts);
/// Check whether \p Path exists. By default checks cached result of \c
/// status(), and falls back on FS if unable to do so.
diff --git a/clang/lib/Frontend/FrontendActions.cpp
b/clang/lib/Frontend/FrontendActions.cpp
index 454653a31534c..eddb2ac0c0834 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -1168,8 +1168,8 @@ void
PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
llvm::SmallVector Tokens;
llvm::SmallVector Directives;
if (scanSourceForDependencyDirectives(
- FromFile.getBuffer(), Tokens, Directives, &CI.getDiagnostics(),
- SM.getLocForStartOfFile(SM.getMainFileID( {
+ FromFile.getBuffer(), Tokens, Directives, CI.getLangOpts(),
+ &CI.getDiagnostics(), SM.getLocForStartOfFile(SM.getMainFileID( {
assert(CI.getDiagnostics().hasErrorOccurred() &&
"no errors reported for failure");
diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp
b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index 0971daa1f3666..f7462aefa4f87 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -62,14 +62,17 @@ struct DirectiveWithTokens {
struct Scanner {
Scanner(StringRef Input,
SmallVectorImpl &Tokens,
- DiagnosticsEngine *Diags, SourceLocation InputSourceLoc)
+ DiagnosticsEngine *Diags, SourceLocation InputSourceLoc,
+ const LangOptions &LangOpts)
: Input(Input), Tokens(Tokens), Diags(Diags),
-InputSourceLoc(InputSourceLoc), LangOpts(getLangOptsForDepScanning()),
+InputSourceLoc(InputSourceLoc),
+LangOpts(getLangOptsForDepScanning(LangOpts)),
TheLexer(InputSourceLoc, LangOpts, Input.begin(), Input.begin(),
Input.end()) {}
- static LangOptions getLangOptsForDepScanning() {
-LangOptions LangOpts;
+ static LangOptions
+ getLangOptsForDepScanning(const LangOptions &invocationLangOpts) {
+LangOptions LangOpts(invocationLangOpts);
// Set the lexer to use 'tok::at' for '@', instead of 'tok::unknown'.
LangOpts.ObjC = true;
LangOpts.LineComment = true;
@@ -700,7 +703,7 @@ bool Scanner::lex_Pragm
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/93753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Pass LangOpts from CompilerInstance to DependencyScanningWorker (PR #93753)
https://github.com/nishithshah2211 created
https://github.com/llvm/llvm-project/pull/93753
This commit fixes https://github.com/llvm/llvm-project/issues/88896 by passing
LangOpts from the CompilerInstance to
DependencyScanningWorker so that the original LangOpts are preserved/respected.
This makes for more accurate parsing/lexing when certain language versions or
features specific to versions are to be used.
>From 01323e5675c5b13014b3b93e7f0c865d7163ed8e Mon Sep 17 00:00:00 2001
From: Nishith Shah
Date: Wed, 29 May 2024 12:34:52 -0700
Subject: [PATCH] Pass LangOpts from CompilerInstance to
DependencyScanningWorker
This commit fixes https://github.com/llvm/llvm-project/issues/88896
by passing LangOpts from the CompilerInstance to
DependencyScanningWorker so that the original LangOpts are
preserved/respected. This makes for more accurate parsing/lexing when
certain language versions or features specific to versions are to be used.
---
.../clang/Lex/DependencyDirectivesScanner.h | 3 ++-
.../DependencyScanningFilesystem.h| 3 ++-
clang/lib/Frontend/FrontendActions.cpp| 4 ++--
clang/lib/Lex/DependencyDirectivesScanner.cpp | 20 +++
.../DependencyScanningFilesystem.cpp | 4 ++--
.../DependencyScanningWorker.cpp | 5 +++--
6 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/clang/include/clang/Lex/DependencyDirectivesScanner.h
b/clang/include/clang/Lex/DependencyDirectivesScanner.h
index 0e115906fbfe5..2f8354dec939f 100644
--- a/clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ b/clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -17,6 +17,7 @@
#ifndef LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
#define LLVM_CLANG_LEX_DEPENDENCYDIRECTIVESSCANNER_H
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/ArrayRef.h"
@@ -117,7 +118,7 @@ struct Directive {
bool scanSourceForDependencyDirectives(
StringRef Input, SmallVectorImpl
&Tokens,
SmallVectorImpl &Directives,
-DiagnosticsEngine *Diags = nullptr,
+const LangOptions &LangOpts, DiagnosticsEngine *Diags = nullptr,
SourceLocation InputSourceLoc = SourceLocation());
/// Print the previously scanned dependency directives as minimized source
text.
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index f7b4510d7f7be..9dc20065a09a3 100644
---
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -363,7 +363,8 @@ class DependencyScanningWorkerFilesystem
///
/// Returns true if the directive tokens are populated for this file entry,
/// false if not (i.e. this entry is not a file or its scan fails).
- bool ensureDirectiveTokensArePopulated(EntryRef Entry);
+ bool ensureDirectiveTokensArePopulated(EntryRef Entry,
+ const LangOptions &LangOpts);
/// Check whether \p Path exists. By default checks cached result of \c
/// status(), and falls back on FS if unable to do so.
diff --git a/clang/lib/Frontend/FrontendActions.cpp
b/clang/lib/Frontend/FrontendActions.cpp
index 454653a31534c..eddb2ac0c0834 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -1168,8 +1168,8 @@ void
PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
llvm::SmallVector Tokens;
llvm::SmallVector Directives;
if (scanSourceForDependencyDirectives(
- FromFile.getBuffer(), Tokens, Directives, &CI.getDiagnostics(),
- SM.getLocForStartOfFile(SM.getMainFileID( {
+ FromFile.getBuffer(), Tokens, Directives, CI.getLangOpts(),
+ &CI.getDiagnostics(), SM.getLocForStartOfFile(SM.getMainFileID( {
assert(CI.getDiagnostics().hasErrorOccurred() &&
"no errors reported for failure");
diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp
b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index 0971daa1f3666..f7462aefa4f87 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -62,14 +62,17 @@ struct DirectiveWithTokens {
struct Scanner {
Scanner(StringRef Input,
SmallVectorImpl &Tokens,
- DiagnosticsEngine *Diags, SourceLocation InputSourceLoc)
+ DiagnosticsEngine *Diags, SourceLocation InputSourceLoc,
+ const LangOptions &LangOpts)
: Input(Input), Tokens(Tokens), Diags(Diags),
-InputSourceLoc(InputSourceLoc), LangOpts(getLangOptsForDepScanning()),
+InputSourceLoc(InputSourceLoc),
+LangOpts(getLangOptsForDepScanning(LangOpts)),
TheLexer(InputSourceLoc, LangOpts, Input.begin(), Input.begin(),
Input.end()) {}
- static LangOptions getLangOptsForDepScanning() {
