[PATCH] D38354: Fix test by using -target instead of cc1 arguments (c-index-test goes through the driver)

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

FYI, I had added -target in this test in r296263 but it was still failing for 
some reason in some bots, that's why I then switched to triple via r296265.
I'm not sure if it's going work now or not for all bots, but it does, that is 
definitely better.


https://reviews.llvm.org/D38354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39217: [libclang, bindings]: add spelling location

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a subscriber: arphaman.
akyrtzi added a comment.

CC'ed @arphaman.


https://reviews.llvm.org/D39217



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49476: [Index] Set OrigD before D is changed.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

`CXIndexDataConsumer.cpp` uses `ASTNode.OrigD`, that code is exercised when you 
run `c-index-test -index-file <...>`. I'd recommend to at least check if that 
command would produce a different output for the test case that you detected 
originally.


Repository:
  rC Clang

https://reviews.llvm.org/D49476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49476: [Index] Set OrigD before D is changed.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi accepted this revision.
akyrtzi added a comment.
This revision is now accepted and ready to land.

Good enough, thanks for looking into this!


Repository:
  rC Clang

https://reviews.llvm.org/D49476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/tools/c-index-test/core_main.cpp:210
+  void *P = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(ProgName, P);
   SmallVector ArgsWithProgName;

Could you move this up to `indextest_core_main` and have `printSourceSymbols()` 
accept the executable path directly ?
This would come in handy to avoid duplication if we later on add another 
function that also needs the executable path.


https://reviews.llvm.org/D50160



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi accepted this revision.
akyrtzi added a comment.
This revision is now accepted and ready to land.

LGTM!


https://reviews.llvm.org/D50160



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33788: Return a canonical path from getClangResourcePath()

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Getting the real path is notoriously slow (at least it's horrible in OSX, not 
sure about linux). Since this is about dropping the '/../' part, could we do 
some simple canonicalization of removing the dots ? Not sure if there is an 
existing function that does that.


https://reviews.llvm.org/D33788



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33788: Return a canonical path from getClangResourcePath()

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

I retract my comment, I see that `getMainExecutable` on OSX calls realpath as 
well, so it's good to use realpath in this code to make sure they are in-sync 
with how the compiler will determine the path.


https://reviews.llvm.org/D33788



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.

This is useful for being able to parse the preprocessor directive blocks even 
the header that defined the macro that they check for hasn't been included.


https://reviews.llvm.org/D34263

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  test/Index/singe-file-parse.m

Index: test/Index/singe-file-parse.m
===
--- test/Index/singe-file-parse.m
+++ test/Index/singe-file-parse.m
@@ -9,3 +9,103 @@
 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -73,6 +73,7 @@
 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+ bool &IncludedUndefinedIds,
  Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track
@@ -93,6 +94,7 @@
   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
@@ -128,6 +130,7 @@
   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)
@@ -255,6 +258,8 @@
 Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
 Result.setIdentifier(II);
 Result.setRange(PeekTok.getLocation());
+DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+   II->getTokenID() != tok::kw_false);
 PP.LexNonComment(PeekTok);
 return false;
   }
@@ -400,7 +405,8 @@
   // Just use DT unmodified as our result.
 } else {
   // Otherwise, we have something like (x+y), and we consumed '(x'.
-  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+   DT.IncludedUndefinedIds, PP))
 return true;
 
   if (PeekTok.isNot(tok::r_paren)) {
@@ -532,6 +538,7 @@
 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Tok

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Hey Vladimir, what you are proposing is orthogonal to this patch. You are 
proposing for "the client to provide the value for an undefined identifier", 
and the patch is about the client not knowing what the value should be so it 
fallbacks to parsing all tokens to get the max amount of info. Note that both 
of the techniques can be combined well, if the client provides the value, the 
preprocessor will take it into account, otherwise if it is stays unresolved it 
will fallback to lexing all tokens.
But what you are proposing is not a replacement for what the patch is doing.


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Lex/PPDirectives.cpp:2774
+// the directive blocks.
+CurPPLexer->pushConditionalLevel(CI.IfLoc, /*wasskip*/false,
+   /*foundnonskip*/true, /*foundelse*/true);

benlangmuir wrote:
> Why is wasSkipping false here?
For `#if` it sets `wasskip` to `true` to indicate later for 
`HandleElseDirective` that the block should not be skipped.
But here (inside `HandleElseDirective`) setting `wasskip` doesn't really affect 
anything, the `HandleEndifDirective` doesn't check it. I chose to set it to 
`false` as indication that the `#else` block does get parsed.


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 102859.
akyrtzi added a comment.

Enhanced doc-comment for the preprocessor option, and fixed indentation on a 
couple of calls.


https://reviews.llvm.org/D34263

Files:
  include/clang/Lex/Preprocessor.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  test/Index/singe-file-parse.m

Index: test/Index/singe-file-parse.m
===
--- test/Index/singe-file-parse.m
+++ test/Index/singe-file-parse.m
@@ -9,3 +9,103 @@
 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -73,6 +73,7 @@
 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+ bool &IncludedUndefinedIds,
  Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track
@@ -93,6 +94,7 @@
   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
@@ -128,6 +130,7 @@
   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)
@@ -255,6 +258,8 @@
 Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
 Result.setIdentifier(II);
 Result.setRange(PeekTok.getLocation());
+DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+   II->getTokenID() != tok::kw_false);
 PP.LexNonComment(PeekTok);
 return false;
   }
@@ -400,7 +405,8 @@
   // Just use DT unmodified as our result.
 } else {
   // Otherwise, we have something like (x+y), and we consumed '(x'.
-  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+   DT.IncludedUndefinedIds, PP))
 return true;
 
   if (PeekTok.isNot(tok::r_paren)) {
@@ -532,6 +538,7 @@
 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
   

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 102885.
akyrtzi added a comment.

Improving doc comment.


https://reviews.llvm.org/D34263

Files:
  include/clang/Lex/Preprocessor.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  test/Index/singe-file-parse.m

Index: test/Index/singe-file-parse.m
===
--- test/Index/singe-file-parse.m
+++ test/Index/singe-file-parse.m
@@ -9,3 +9,103 @@
 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -73,6 +73,7 @@
 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+ bool &IncludedUndefinedIds,
  Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track
@@ -93,6 +94,7 @@
   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
@@ -128,6 +130,7 @@
   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)
@@ -255,6 +258,8 @@
 Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
 Result.setIdentifier(II);
 Result.setRange(PeekTok.getLocation());
+DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+   II->getTokenID() != tok::kw_false);
 PP.LexNonComment(PeekTok);
 return false;
   }
@@ -400,7 +405,8 @@
   // Just use DT unmodified as our result.
 } else {
   // Otherwise, we have something like (x+y), and we consumed '(x'.
-  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+   DT.IncludedUndefinedIds, PP))
 return true;
 
   if (PeekTok.isNot(tok::r_paren)) {
@@ -532,6 +538,7 @@
 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In https://reviews.llvm.org/D34263#783694, @klimek wrote:

> how many patches for single file mode are coming down the road, though? I'm 
> somewhat concerned about the overall complexity it'll add to clang.


There is no other patch for using this preprocessor option. Any related 
improvements down the road will be about general improvements for error 
recovery, for example things like this:

- Introduce an `UnresolvedTypename` type instead of changing unresolved types 
to `int`.
- For `@interace A : B`, don't completely drop `B` from the super-class list of 
`A` if it is unresolved.

These kind of improvements are not conditional.


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In https://reviews.llvm.org/D34263#783770, @voskresensky.vladimir wrote:

> What I find problematic in this patch is PPOpts->SingleFileParseMode checks.
>  Let's suppose we implement what I mentioned above => how is it going to 
> co-exist nicely? I think code will be less understandable with both: check of 
> flag and call to PPCallbacks.
>  What I propose is to move the logic from the PPOpts single flag into 
> PPCallbacks. And from check of flag to query of PPCallbacks.
>  Of course when you create PPCallbacks you can consult global 
> SingleFileParseMode to create default implementation to answer "any symbol is 
> defined", so you get your use-case easily handled, but it also gives others 
> more flexibility.


There is a bit of misunderstanding on what this patch is doing, the patch is 
not about answering "any symbol is defined". See my cfe-dev email 
(http://lists.llvm.org/pipermail/cfe-dev/2017-June/054254.html) that provides 
some more context.
The patch is doing these 2 things:

- Keep track of whether a preprocessor directive condition used a symbol that 
was not resolved at all
- If above is true then make the preprocessor parse all directive blocks, not 
arbitrary choose one of them.

Here's an example to clarify the difference:

Say that you have this code:

  #if TARGET_IOS
  @interface A @end
  #else
  @interface B @end
  #endif



1. Without any changes this will happen:
  - TARGET_IOS is unresolved so it will be treated as `0`
  - `#if` block will be skipped, `#else` will be parsed
  - AST will contain `@interface B`

2. With adding a PPCallback that answers "any symbol is defined" (what you are 
proposing):
  - TARGET_IOS is undefined but the PPCallback sets it to `1`
  - `#if` block will be parsed, `#else` will be skipped
  - AST will contain `@interface A`

3. With this patch and enabling `SingleFileParseMode`
  - TARGET_IOS is unresolved
  - Both `#if` and `#else` blocks will be parsed
  - AST will contain both `@interface A` and `@interface B`

As you can see 2. and 3. above are not overlapping so they can "co-exist 
nicely" like this:

- TARGET_IOS is undefined, ask the PPCallback if it has a value for it or not
  - If the callback provides a value then parse as 1. or 2. depending on the 
value
  - If the callback cannot provide a specific value then parse as 3.


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 103066.
akyrtzi added a comment.

Provide doc-comments for `struct DirectiveEvalResult`.


https://reviews.llvm.org/D34263

Files:
  include/clang/Lex/Preprocessor.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  test/Index/singe-file-parse.m

Index: test/Index/singe-file-parse.m
===
--- test/Index/singe-file-parse.m
+++ test/Index/singe-file-parse.m
@@ -9,3 +9,103 @@
 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -73,6 +73,7 @@
 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+ bool &IncludedUndefinedIds,
  Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track
@@ -93,6 +94,7 @@
   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
@@ -128,6 +130,7 @@
   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)
@@ -255,6 +258,8 @@
 Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
 Result.setIdentifier(II);
 Result.setRange(PeekTok.getLocation());
+DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+   II->getTokenID() != tok::kw_false);
 PP.LexNonComment(PeekTok);
 return false;
   }
@@ -400,7 +405,8 @@
   // Just use DT unmodified as our result.
 } else {
   // Otherwise, we have something like (x+y), and we consumed '(x'.
-  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+   DT.IncludedUndefinedIds, PP))
 return true;
 
   if (PeekTok.isNot(tok::r_paren)) {
@@ -532,6 +538,7 @@
 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool 

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 103067.
akyrtzi added a comment.

Update doc-comment for `Preprocessor::EvaluateDirectiveExpression`


https://reviews.llvm.org/D34263

Files:
  include/clang/Lex/Preprocessor.h
  include/clang/Lex/PreprocessorOptions.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  test/Index/singe-file-parse.m

Index: test/Index/singe-file-parse.m
===
--- test/Index/singe-file-parse.m
+++ test/Index/singe-file-parse.m
@@ -9,3 +9,103 @@
 // CHECK: [[@LINE+1]]:8: ObjCInstanceMethodDecl=some_meth
 -(void)some_meth;
 @end
+
+#if 1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test1
+@interface Test1 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test2 @end
+#endif
+
+#if 0
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test3 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test4
+@interface Test4 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test5
+@interface Test5 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test6
+@interface Test6 @end
+#endif
+
+#define SOMETHING_DEFINED 1
+#if SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test7
+@interface Test7 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test8 @end
+#endif
+
+#if defined(SOMETHING_NOT_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test9
+@interface Test9 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test10
+@interface Test10 @end
+#endif
+
+#if defined(SOMETHING_DEFINED)
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test11
+@interface Test11 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test12 @end
+#endif
+
+#if SOMETHING_NOT_DEFINED1
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test13
+@interface Test13 @end
+#elif SOMETHING_NOT_DEFINED2
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test14
+@interface Test14 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test15
+@interface Test15 @end
+#endif
+
+#ifdef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test19
+@interface Test19 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test20
+@interface Test20 @end
+#endif
+
+#ifdef SOMETHING_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test21
+@interface Test21 @end
+#else
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test22 @end
+#endif
+
+#ifndef SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test23
+@interface Test23 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test24
+@interface Test24 @end
+#endif
+
+#ifndef SOMETHING_DEFINED
+// CHECK-NOT: [[@LINE+1]]:12:
+@interface Test25 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test26
+@interface Test26 @end
+#endif
+
+#if 1 < SOMETHING_NOT_DEFINED
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test27
+@interface Test27 @end
+#else
+// CHECK: [[@LINE+1]]:12: ObjCInterfaceDecl=Test28
+@interface Test28 @end
+#endif
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -73,6 +73,7 @@
 
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &PeekTok, bool ValueLive,
+ bool &IncludedUndefinedIds,
  Preprocessor &PP);
 
 /// DefinedTracker - This struct is used while parsing expressions to keep track
@@ -93,6 +94,7 @@
   /// TheMacro - When the state is DefinedMacro or NotDefinedMacro, this
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
+  bool IncludedUndefinedIds = false;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
@@ -128,6 +130,7 @@
   MacroDefinition Macro = PP.getMacroDefinition(II);
   Result.Val = !!Macro;
   Result.Val.setIsUnsigned(false); // Result is signed intmax_t.
+  DT.IncludedUndefinedIds = !Macro;
 
   // If there is a macro, mark it used.
   if (Result.Val != 0 && ValueLive)
@@ -255,6 +258,8 @@
 Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
 Result.setIdentifier(II);
 Result.setRange(PeekTok.getLocation());
+DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
+   II->getTokenID() != tok::kw_false);
 PP.LexNonComment(PeekTok);
 return false;
   }
@@ -400,7 +405,8 @@
   // Just use DT unmodified as our result.
 } else {
   // Otherwise, we have something like (x+y), and we consumed '(x'.
-  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive, PP))
+  if (EvaluateDirectiveSubExpr(Result, 1, PeekTok, ValueLive,
+   DT.IncludedUndefinedIds, PP))
 return true;
 
   if (PeekTok.isNot(tok::r_paren)) {
@@ -532,6 +538,7 @@
 /// evaluation, such as division by zero warnings.
 static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
  Token &Pe

[PATCH] D41514: [Index] Reduce size of SymbolInfo struct.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Nice one! One minor change I'd suggest is to change `SymbolProperty` to `enum 
class SymbolProperty : SymbolPropertySet`, so that if it needs to increase we 
only change the type in one place.


Repository:
  rC Clang

https://reviews.llvm.org/D41514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41514: [Index] Reduce size of SymbolInfo struct.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi accepted this revision.
akyrtzi added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D41514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41575: [index] Return when DC is null in handleReference

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Could you add a test case for this change ? See examples in `test/Index/Core`.
Also for the test case code: `template  class Actor = 
actor>`, is the `actor` reference in this code reported ?
See the test examples on how to print out and test how the source symbols are 
indexed.


Repository:
  rC Clang

https://reviews.llvm.org/D41575



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41575: [index] Return when DC is null in handleReference

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Ah, sorry I mislead you. To test this try using `c-index-test -index-file 
/path/to/file`, see other examples in `test/Index`, e.g. 
`test/Index/index-file.cpp`


Repository:
  rL LLVM

https://reviews.llvm.org/D41575



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39050: Add index-while-building support to Clang

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Hey Marc,

> The fact that both clang and clangd have to agree on the format so that 
> index-while-building can be used seems to make it inherently not possible to 
> extend

I don't think "not possible to extend" is quite correct, we can make it so that 
the format allows optional data to be recorded.

On the topic of recording the end-loc, I agree it's not much data overall, but 
it will be useful to examine the uses closely and to figure out whether it's 
really required and whether it is at the same time inadequate for other uses.

>   I changed my prototype so that the end-loc is not stored in the index but 
> rather computed "on the fly" using SourceManager and Lexer only.

I assume you used SingleFileParseMode+SkipFunctionBodies for this, right ?

> For my little benchmark, I used the LLVM/Clang/Clangd code base which I 
> queried for all references of "std" (the namespace) which is around 46K 
> references in the index.

This is an interesting use case, and I can say we have some experience because 
Xcode has this functionality without requiring the end-loc for every reference.
So what it does is that it has a 'name' to look for (say 'foo' for  the 
variable foo) and if it finds the name in the location then it highlights, 
otherwise if it doesn't find it (e.g. because it is an implicit reference) then 
it points to the location but doesn't highlight something. The same thing 
happens for operator overloads (the operators get highlighted at the reference 
location).
For implicit references it's most likely there's nothing to highlight so the 
end-loc will most likely be empty anyway (or same as start-loc ?) to indicate 
an empty range.

> With SingleFileParseMode, I get several errors:

Good point, the parser definitely needs recovery improvements in C++.

> With SkipFunctionBodies alone, I can get the Decl* but 
> FunctionDecl::getSourceRange() doesn't include the body

This seems strange, there's an EndRangeLoc field that should have been filled 
in, not exactly sure if it is a bug or omission.

Going back to the topic of what use cases end-loc covers, note that it still 
seems inadequate for peek-definition functionality. You can't set it to 
body-end loc (otherwise occurrences highlighting would highlight the whole body 
which I think is undesirable) and you still need to include doc-comments if 
they exist.


https://reviews.llvm.org/D39050



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39050: Add index-while-building support to Clang

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

> That would be good. How would one go about asking Clang to generate this 
> extra information? Would a Clang Plugin be suitable for this?

That's an interesting idea that we could explore, but I don't have much 
experience with that mechanism to comment on.

> Only the lexer was needed to get the end of the token

Ok, that's interesting, not sure why Xcode is so fast to highlight, did you 
reuse same SourceManager/Lexer/buffers for occurrences from same file ? We'd 
definitely add the end-loc if we cannot come up with a mechanism to highlight 
fast enough without it.

> I think it's useful to highlight something even when the name is not there. 
> For example in "MyClass o1, o2;" it feels natural that o1 and o2 would get 
> highlighted.

To clarify, even with implicit references the start loc points to something. In 
this case the implicit references can have start locs for the o1 and o2 
identifiers and the end result for the UI will be the same (o1 and o2 get 
highlighted) even without having end-locs for all references.

> It does? I can only seem to do a textual search.

The example I tried is the following. If you could file a bug report for the 
test case that did not work as you expected it would be much appreciated!

  class Something1 {
  public:
  Something1() {}
  ~Something1() {}
  operator int() {
  return 0;
  }
  
  friend int operator <<(Something1 &p, Something1 &p2) {
  return 0;
  }
  };
  
  void foo1(Something1 p1, Something1 p2) {
  p1 << p2;
  p1 << p2;
  }



> here the "operator std::string" would be called and MyStringRef could be 
> highlighted

Even without end-loc, the start loc could point to MyStringRef and you could 
highlight it.


https://reviews.llvm.org/D39050



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

It's not clear to me what kind of issue you are addressing, for example is the 
unit test hitting an assertion hit without your changes ? Or is there something 
else ?
Also it would be good to add some documentation comments to clarify what's the 
use case and when `ASTUnit::beginSourceFile()` should be getting called.


Repository:
  rC Clang

https://reviews.llvm.org/D47445



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Thanks for the explanation. Please do add documentation comments for the new 
method so people using ASTUnit in their own code have an idea when and why they 
would need to call this. Something like "if you intend to emit additional 
diagnostics after the ASTUnit is created [...]". 
Also consider making the naming more clear to match the intended purpose, like 
`enableEmittingAdditionalDiagnostics()` or something similar.
Otherwise LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D47445



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

We could leave `disableSourceFileDiagnostics` off until someone finds a use 
case for it.


Repository:
  rC Clang

https://reviews.llvm.org/D47445



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

rjmccall wrote:
> Paging @akyrtzi here.  The address-space qualifier should be part of the USR 
> but I don't think if there's a defined schema for that.
If I understand correctly from other comments you can't add special mangling 
and USR generation yet and intend to add FIXME for doing mangling support later 
? If yes, could you also add FIXME here and re-visit ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: lib/Index/USRGeneration.cpp:274
+if (unsigned quals = MD->getTypeQualifiers().getCVRUQualifiers())
   Out << (char)('0' + quals);
 switch (MD->getRefQualifier()) {

rjmccall wrote:
> akyrtzi wrote:
> > rjmccall wrote:
> > > Paging @akyrtzi here.  The address-space qualifier should be part of the 
> > > USR but I don't think if there's a defined schema for that.
> > If I understand correctly from other comments you can't add special 
> > mangling and USR generation yet and intend to add FIXME for doing mangling 
> > support later ? If yes, could you also add FIXME here and re-visit ?
> While you're here, can you described the right way for us to extend USR 
> generation here to potentially add an address space?
I'm not familiar with the mechanism, is address space supposed to be identified 
via a single integer ? If so, then I think adding '-' character plus the 
integer as character for the address space, immediately after the type 
qualifiers, should be a way to go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39050: Add index-while-building support to Clang

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: include/clang/Frontend/FrontendOptions.h:380
+  /// Whether to ignore system files when writing out index data
+  unsigned IndexIgnoreSystemSymbols : 1;
+  /// Whether to include the codegen name of symbols in the index data

gribozavr wrote:
> Would it make more sense to flip this boolean to positive?  
> "IndexIncludeSystemSymbols"?
@jkorous I noticed this name can be misleading, it may seem as if what this 
does is "avoid indexing system symbol occurrences" but what it actually does is 
"avoid indexing symbol occurrences from system files". We should rename it to 
"IndexIgnoreSystemHeaders" or "IndexIncludeSystemHeaders" per Dmitri's 
suggestion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D39050/new/

https://reviews.llvm.org/D39050



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> jkorous wrote:
> > mgorny wrote:
> > > Why? I suppose this deserves a comment.
> > I'll add this comment:
> > 
> > // The file might have been removed just after we received the event.
> Wouldn't that cause removals to be reported twice?
Not quite sure if it can happen in practice but I'd suggest to accept this as 
potential occurrence and add it to documentation ("a 'removed' event may be 
reported twice). I think it is better to handle a definite "fact" (the file 
doesn't exist) than ignore it and assume the 'removed' event will eventually 
show up, or try to eliminate the double reporting which would over-complicate 
the implementation.

After all, if inotify() is not 100% reliable then there's already the 
possibility that you'll get a 'removed' event for a file that was not reported 
as 'added' before.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > jkorous wrote:
> > > > mgorny wrote:
> > > > > Why? I suppose this deserves a comment.
> > > > I'll add this comment:
> > > > 
> > > > // The file might have been removed just after we received the event.
> > > Wouldn't that cause removals to be reported twice?
> > Not quite sure if it can happen in practice but I'd suggest to accept this 
> > as potential occurrence and add it to documentation ("a 'removed' event may 
> > be reported twice). I think it is better to handle a definite "fact" (the 
> > file doesn't exist) than ignore it and assume the 'removed' event will 
> > eventually show up, or try to eliminate the double reporting which would 
> > over-complicate the implementation.
> > 
> > After all, if inotify() is not 100% reliable then there's already the 
> > possibility that you'll get a 'removed' event for a file that was not 
> > reported as 'added' before.
> I see this as a partial workaround for race condition. You 'fix' it if 
> removal happens between inotify reporting the event and you returning it. You 
> don't if removal happens after you push it to events. Therefore, the caller 
> still needs to account for 'added' event being obsolete. I don't really see a 
> purpose in trying to workaround the problem partially here if the caller 
> still needs to account for it. Effectively, you're replacing one normal case 
> and one corner case with one normal case and two corner cases.
I was mainly pointing out that the client already has to be prepared for a 
'removed' event that does not have an associated 'added' event, regardless of 
what this code is doing. Therefore a potential double 'removed' event doesn't 
add complexity to the client.

Could you clarify how the code should handle the inability to get the mod time 
? Should it ignore the event ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > jkorous wrote:
> > > > > > mgorny wrote:
> > > > > > > Why? I suppose this deserves a comment.
> > > > > > I'll add this comment:
> > > > > > 
> > > > > > // The file might have been removed just after we received the 
> > > > > > event.
> > > > > Wouldn't that cause removals to be reported twice?
> > > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > > this as potential occurrence and add it to documentation ("a 'removed' 
> > > > event may be reported twice). I think it is better to handle a definite 
> > > > "fact" (the file doesn't exist) than ignore it and assume the 'removed' 
> > > > event will eventually show up, or try to eliminate the double reporting 
> > > > which would over-complicate the implementation.
> > > > 
> > > > After all, if inotify() is not 100% reliable then there's already the 
> > > > possibility that you'll get a 'removed' event for a file that was not 
> > > > reported as 'added' before.
> > > I see this as a partial workaround for race condition. You 'fix' it if 
> > > removal happens between inotify reporting the event and you returning it. 
> > > You don't if removal happens after you push it to events. Therefore, the 
> > > caller still needs to account for 'added' event being obsolete. I don't 
> > > really see a purpose in trying to workaround the problem partially here 
> > > if the caller still needs to account for it. Effectively, you're 
> > > replacing one normal case and one corner case with one normal case and 
> > > two corner cases.
> > I was mainly pointing out that the client already has to be prepared for a 
> > 'removed' event that does not have an associated 'added' event, regardless 
> > of what this code is doing. Therefore a potential double 'removed' event 
> > doesn't add complexity to the client.
> > 
> > Could you clarify how the code should handle the inability to get the mod 
> > time ? Should it ignore the event ?
> Given the code is supposed to wrap filesystem notification layer, I'd say it 
> should pass the events unmodified and not double-guess what the client 
> expects. The client needs to be prepared for non-atomicity of this anyway.
So it should report an 'added' event but with optional mod-time, essentially 
reporting that something was added that doesn't exist ?
I'd prefer not to do that because it complicates the client without any real 
benefit. It was great that you pointed out this part of the code but I'd 
recommend that if the file is gone we should ignore the 'added' event, instead 
of complicating the API for a corner case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > akyrtzi wrote:
> > > > > > mgorny wrote:
> > > > > > > jkorous wrote:
> > > > > > > > mgorny wrote:
> > > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > > I'll add this comment:
> > > > > > > > 
> > > > > > > > // The file might have been removed just after we received the 
> > > > > > > > event.
> > > > > > > Wouldn't that cause removals to be reported twice?
> > > > > > Not quite sure if it can happen in practice but I'd suggest to 
> > > > > > accept this as potential occurrence and add it to documentation ("a 
> > > > > > 'removed' event may be reported twice). I think it is better to 
> > > > > > handle a definite "fact" (the file doesn't exist) than ignore it 
> > > > > > and assume the 'removed' event will eventually show up, or try to 
> > > > > > eliminate the double reporting which would over-complicate the 
> > > > > > implementation.
> > > > > > 
> > > > > > After all, if inotify() is not 100% reliable then there's already 
> > > > > > the possibility that you'll get a 'removed' event for a file that 
> > > > > > was not reported as 'added' before.
> > > > > I see this as a partial workaround for race condition. You 'fix' it 
> > > > > if removal happens between inotify reporting the event and you 
> > > > > returning it. You don't if removal happens after you push it to 
> > > > > events. Therefore, the caller still needs to account for 'added' 
> > > > > event being obsolete. I don't really see a purpose in trying to 
> > > > > workaround the problem partially here if the caller still needs to 
> > > > > account for it. Effectively, you're replacing one normal case and one 
> > > > > corner case with one normal case and two corner cases.
> > > > I was mainly pointing out that the client already has to be prepared 
> > > > for a 'removed' event that does not have an associated 'added' event, 
> > > > regardless of what this code is doing. Therefore a potential double 
> > > > 'removed' event doesn't add complexity to the client.
> > > > 
> > > > Could you clarify how the code should handle the inability to get the 
> > > > mod time ? Should it ignore the event ?
> > > Given the code is supposed to wrap filesystem notification layer, I'd say 
> > > it should pass the events unmodified and not double-guess what the client 
> > > expects. The client needs to be prepared for non-atomicity of this anyway.
> > So it should report an 'added' event but with optional mod-time, 
> > essentially reporting that something was added that doesn't exist ?
> > I'd prefer not to do that because it complicates the client without any 
> > real benefit. It was great that you pointed out this part of the code but 
> > I'd recommend that if the file is gone we should ignore the 'added' event, 
> > instead of complicating the API for a corner case.
> Except that is technically impossible to avoid reporting something that 
> doesn't exist because it can be removed just after you check for it. So the 
> client needs to *always* support it, otherwise it's fragile to race 
> conditions.
> 
> This extra check just covers the short period (-> 0) between reporting and 
> checking. It's needless complexity that doesn't solve the problem. If it does 
> anything, then it gives you false security that you've solved the problem 
> when actually the file may still disappear 1 ns after you've checked that it 
> existed.
Ok, that's fair, @jkorous I'm fine with removing the mod-time from the 
DirectoryWatcher API. We can get and report the mod-time at the index-store 
library layer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58418/new/

https://reviews.llvm.org/D58418



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65846: Improve error message from FrontendAction

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Sorry, it's not clear to me what is the issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65846/new/

https://reviews.llvm.org/D65846



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65846: Improve error message from FrontendAction

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D65846#1619645 , @jfb wrote:

> My current guess is that this part of the test:
>
>   c-index-test -write-pch %t.h.pch %s -fmodules -fmodules-cache-path=%t.mcp 
> -Xclang -triple -Xclang x86_64-apple-darwin
>
>
> Is expected to generate the `unknown type name` error, but when than happens 
> it ignores `-fmodules-cache-path=%t.mcp` and doesn't create the directory. 
> The next line expects the directory to exist, which is why it can't create 
> the lock file (because the directory it's trying to create it in doesn't 
> exist).


`clang -fmodules -fmodules-cache-path=...` is supposed to create the directory 
for the cache path, including the parent directories, AFAIK. If this doesn't 
happen it is a behavior change (and undesirable IMO).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65846/new/

https://reviews.llvm.org/D65846



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65846: Improve error message from FrontendAction

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D65846#1619752 , @bruno wrote:

> > `clang -fmodules -fmodules-cache-path=...` is supposed to create the 
> > directory for the cache path, including the parent directories, AFAIK. If 
> > this doesn't happen it is a behavior change (and undesirable IMO).
>
> Is `c-index-test` invoking `clang` or do we just have a similar interface? 
> Perhaps it's not doing it right (haven't seen this problem happening directly 
> while invoking clang). Should `-fallow-pch-with-compiler-errors` be 
> considered somehow here?


To clarify, `c-index-test` only uses the libclang APIs. JF said "The next line 
expects the directory to exist", which I assume refers to the `clang` 
invocation, which is why I mention that a clang invocation does not need to 
expect for the cache directory to exist.
Also `-fallow-pch-with-compiler-errors` is always enabled by the libclang API 
that is used to create a PCH.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65846/new/

https://reviews.llvm.org/D65846



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49476: [Index] Set OrigD before D is changed.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Is it possible to add a regression test case ? I assume this is fixing some 
issue, we should make sure we don't regress again.


Repository:
  rC Clang

https://reviews.llvm.org/D49476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39050: Add index-while-building support to Clang

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

@malaperle, to clarify we are not suggesting that you write your own parser, 
the suggestion is to use clang in 'fast-scan' mode to get the structure of the 
declarations of a single file, see `CXTranslationUnit_SingleFileParse` (along 
with enabling skipping of bodies). We have found clang is super fast when you 
only try to get the structure of a file like this. We can make convenient APIs 
to provide the syntactic structure of declarations based on their location.

But let's say we added the end-loc, is it enough ? If you want to implement the 
'peek the definition' like Eclipse, then it is not enough, you also need to 
figure out if there are documentation comments associated with the declaration 
and also show those. Also what if you want to highlight the type signature of a 
function, then just storing the location of the closing brace of its body is 
not enough. There can be any arbitrary things you may want to get from the 
structure of the declaration (e.g. the parameter ranges), but we could provide 
an API to gather any syntactic structure info you may want.

I would encourage you to try 
`CXTranslationUnit_SingleFileParse|CXTranslationUnit_SkipFunctionBodies`, you 
will be pleasantly surprised with how fast this mode is. The `c-index-test` 
option is `-single-file-parse`.


https://reviews.llvm.org/D39050



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132801

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -80,3 +80,6 @@
 
 // RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | 
FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
 // VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+
+// RUN: %clang -fno-modules -fmodule-map-file=module.modulemap -### %s 2>&1 | 
FileCheck -check-prefix=MODULE_MAP_FILE %s
+// MODULE_MAP_FILE-NOT: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3726,25 +3726,29 @@
  options::OPT_fno_modules_validate_input_files_content,
  false))
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
-  }
-
-  // -fmodule-name specifies the module that is currently being built (or
-  // used for header checking by -fmodule-maps).
-  Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
 
-  // -fmodule-map-file can be used to specify files containing module
-  // definitions.
-  Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
-
-  // -fbuiltin-module-map can be used to load the clang
-  // builtin headers modulemap file.
-  if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
-SmallString<128> BuiltinModuleMap(D.ResourceDir);
-llvm::sys::path::append(BuiltinModuleMap, "include");
-llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
-if (llvm::sys::fs::exists(BuiltinModuleMap))
-  CmdArgs.push_back(
-  Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+// -fmodule-name specifies the module that is currently being built (or
+// used for header checking by -fmodule-maps).
+Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
+
+// -fmodule-map-file can be used to specify files containing module
+// definitions.
+Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
+
+// -fbuiltin-module-map can be used to load the clang
+// builtin headers modulemap file.
+if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
+  SmallString<128> BuiltinModuleMap(D.ResourceDir);
+  llvm::sys::path::append(BuiltinModuleMap, "include");
+  llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
+  if (llvm::sys::fs::exists(BuiltinModuleMap))
+CmdArgs.push_back(
+Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+}
+  } else {
+Args.ClaimAllArgs(options::OPT_fmodule_name_EQ);
+Args.ClaimAllArgs(options::OPT_fmodule_map_file);
+Args.ClaimAllArgs(options::OPT_fbuiltin_module_map);
   }
 
   // The -fmodule-file== form specifies the mapping of module


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -80,3 +80,6 @@
 
 // RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
 // VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+
+// RUN: %clang -fno-modules -fmodule-map-file=module.modulemap -### %s 2>&1 | FileCheck -check-prefix=MODULE_MAP_FILE %s
+// MODULE_MAP_FILE-NOT: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3726,25 +3726,29 @@
  options::OPT_fno_modules_validate_input_files_content,
  false))
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
-  }
-
-  // -fmodule-name specifies the module that is currently being built (or
-  // used for header checking by -fmodule-maps).
-  Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
 
-  // -fmodule-map-file can be used to specify files containing module
-  // definitions.
-  Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
-
-  // -fbuiltin-module-map can be used to load the clang
-  // builtin headers modulemap file.
-  if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
-SmallString<128> BuiltinModuleMap(D.ResourceDir);
-llvm::sys::path::append(BuiltinModuleMap, "include");
-llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
-if (llvm::sys::fs::exists(BuiltinModuleMap))
-  CmdArgs.push_back(
-  Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+// -fmodule-name specif

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 456483.
akyrtzi added a comment.

Merge the new `RUN` line together with the prior 2.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132801/new/

https://reviews.llvm.org/D132801

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -75,8 +75,8 @@
 // RUN: %clang -fno-modules -fbuild-session-timestamp=123 -### %s 2>&1 | 
FileCheck -check-prefix=SESSION_FLAG %s
 // SESSION_FLAG-NOT: -fbuild-session-timestamp
 
-// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -### %s 
2>&1 | FileCheck -check-prefix=VALIDATE_ONCE_FLAG %s
-// VALIDATE_ONCE_FLAG-NOT: -fmodules-validate-once-per-build-session
-
-// RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | 
FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
-// VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session 
-fmodules-validate-system-headers -fmodule-map-file=module.modulemap \
+// RUN:   -### %s 2>&1 | FileCheck -check-prefix=IGNORED_FLAGS %s
+// IGNORED_FLAGS-NOT: -fmodules-validate-once-per-build-session
+// IGNORED_FLAGS-NOT: -fmodules-validate-system-headers
+// IGNORED_FLAGS-NOT: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3726,25 +3726,29 @@
  options::OPT_fno_modules_validate_input_files_content,
  false))
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
-  }
-
-  // -fmodule-name specifies the module that is currently being built (or
-  // used for header checking by -fmodule-maps).
-  Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
 
-  // -fmodule-map-file can be used to specify files containing module
-  // definitions.
-  Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
-
-  // -fbuiltin-module-map can be used to load the clang
-  // builtin headers modulemap file.
-  if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
-SmallString<128> BuiltinModuleMap(D.ResourceDir);
-llvm::sys::path::append(BuiltinModuleMap, "include");
-llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
-if (llvm::sys::fs::exists(BuiltinModuleMap))
-  CmdArgs.push_back(
-  Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+// -fmodule-name specifies the module that is currently being built (or
+// used for header checking by -fmodule-maps).
+Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
+
+// -fmodule-map-file can be used to specify files containing module
+// definitions.
+Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
+
+// -fbuiltin-module-map can be used to load the clang
+// builtin headers modulemap file.
+if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
+  SmallString<128> BuiltinModuleMap(D.ResourceDir);
+  llvm::sys::path::append(BuiltinModuleMap, "include");
+  llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
+  if (llvm::sys::fs::exists(BuiltinModuleMap))
+CmdArgs.push_back(
+Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+}
+  } else {
+Args.ClaimAllArgs(options::OPT_fmodule_name_EQ);
+Args.ClaimAllArgs(options::OPT_fmodule_map_file);
+Args.ClaimAllArgs(options::OPT_fbuiltin_module_map);
   }
 
   // The -fmodule-file== form specifies the mapping of module


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -75,8 +75,8 @@
 // RUN: %clang -fno-modules -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=SESSION_FLAG %s
 // SESSION_FLAG-NOT: -fbuild-session-timestamp
 
-// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=VALIDATE_ONCE_FLAG %s
-// VALIDATE_ONCE_FLAG-NOT: -fmodules-validate-once-per-build-session
-
-// RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
-// VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -fmodules-validate-system-headers -fmodule-map-file=module.modulemap \
+// RUN:   -### %s 2>&1 | FileCheck -check-prefix=IGNORED_FLAGS %s
+// IGNORED_FLAGS-NOT: -fmodules-validate-once-per-build-session
+// IGNORED_FLAGS-NOT: -fmodules-validate-system-headers
+// IGNORED_FLAGS-NOT: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi marked an inline comment as done.
akyrtzi added inline comments.



Comment at: clang/test/Driver/modules.m:81
 
 // RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | 
FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
 // VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers

MaskRay wrote:
> You may combine `-fmodule-map-file=module.modulemap` with this RUN line to 
> avoid introducing a new RUN line (every run makes testsuite slightly slower).
Good suggestion, I merged the last existing 2 `RUN` lines along with the new 
one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132801/new/

https://reviews.llvm.org/D132801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
akyrtzi marked an inline comment as done.
Closed by commit rG33162a81d4c9: [driver] Additional ignoring of module-map 
related flags, if modules are… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132801/new/

https://reviews.llvm.org/D132801

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -75,8 +75,8 @@
 // RUN: %clang -fno-modules -fbuild-session-timestamp=123 -### %s 2>&1 | 
FileCheck -check-prefix=SESSION_FLAG %s
 // SESSION_FLAG-NOT: -fbuild-session-timestamp
 
-// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -### %s 
2>&1 | FileCheck -check-prefix=VALIDATE_ONCE_FLAG %s
-// VALIDATE_ONCE_FLAG-NOT: -fmodules-validate-once-per-build-session
-
-// RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | 
FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
-// VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session 
-fmodules-validate-system-headers -fmodule-map-file=module.modulemap \
+// RUN:   -### %s 2>&1 | FileCheck -check-prefix=IGNORED_FLAGS %s
+// IGNORED_FLAGS-NOT: -fmodules-validate-once-per-build-session
+// IGNORED_FLAGS-NOT: -fmodules-validate-system-headers
+// IGNORED_FLAGS-NOT: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3726,25 +3726,29 @@
  options::OPT_fno_modules_validate_input_files_content,
  false))
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
-  }
-
-  // -fmodule-name specifies the module that is currently being built (or
-  // used for header checking by -fmodule-maps).
-  Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
 
-  // -fmodule-map-file can be used to specify files containing module
-  // definitions.
-  Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
-
-  // -fbuiltin-module-map can be used to load the clang
-  // builtin headers modulemap file.
-  if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
-SmallString<128> BuiltinModuleMap(D.ResourceDir);
-llvm::sys::path::append(BuiltinModuleMap, "include");
-llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
-if (llvm::sys::fs::exists(BuiltinModuleMap))
-  CmdArgs.push_back(
-  Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+// -fmodule-name specifies the module that is currently being built (or
+// used for header checking by -fmodule-maps).
+Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
+
+// -fmodule-map-file can be used to specify files containing module
+// definitions.
+Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
+
+// -fbuiltin-module-map can be used to load the clang
+// builtin headers modulemap file.
+if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
+  SmallString<128> BuiltinModuleMap(D.ResourceDir);
+  llvm::sys::path::append(BuiltinModuleMap, "include");
+  llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
+  if (llvm::sys::fs::exists(BuiltinModuleMap))
+CmdArgs.push_back(
+Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+}
+  } else {
+Args.ClaimAllArgs(options::OPT_fmodule_name_EQ);
+Args.ClaimAllArgs(options::OPT_fmodule_map_file);
+Args.ClaimAllArgs(options::OPT_fbuiltin_module_map);
   }
 
   // The -fmodule-file== form specifies the mapping of module


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -75,8 +75,8 @@
 // RUN: %clang -fno-modules -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=SESSION_FLAG %s
 // SESSION_FLAG-NOT: -fbuild-session-timestamp
 
-// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=VALIDATE_ONCE_FLAG %s
-// VALIDATE_ONCE_FLAG-NOT: -fmodules-validate-once-per-build-session
-
-// RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
-// VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -fmodules-validate-system-headers -fmodule-map-file=module.modulemap \
+// RUN:   -### %s 2>&1 | FileCheck -check-prefix=IGNORED_FLAGS %s
+// IGNORED_FLAGS-NOT: -fmodules-validate-once-per-build-session
+// IGNORED_FLAGS-NOT: -fmodules-validate-system-headers
+// IGNORED_

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D132801#3760014 , @rsmith wrote:

> This doesn't look right to me -- we still use module maps when modules are 
> disabled to enforce layering checking, and when 
> `-fmodules-local-submodule-visibility` is enabled but `-fmodules` is disabled 
> we'll use them to provide modular semantics without pre-building modules. I'm 
> going to revert.

Sorry, I wasn't aware of that. Could `-fmodule-map-file=` be pruned out when 
`-fmodules-local-submodule-visibility` or `-fmodules-decluse` are not enabled?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132801/new/

https://reviews.llvm.org/D132801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133229: [driver] Prune module-map related flags, if they are not going to be needed

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

This is follow-up from https://reviews.llvm.org/D132801, but taking into 
account the conditions
where the module-map flags are still used even when `-fmodules` is disabled.

This useful for removing unnecessary input dependencies from the cc1 invocation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133229

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m


Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -75,8 +75,13 @@
 // RUN: %clang -fno-modules -fbuild-session-timestamp=123 -### %s 2>&1 | 
FileCheck -check-prefix=SESSION_FLAG %s
 // SESSION_FLAG-NOT: -fbuild-session-timestamp
 
-// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session -### %s 
2>&1 | FileCheck -check-prefix=VALIDATE_ONCE_FLAG %s
-// VALIDATE_ONCE_FLAG-NOT: -fmodules-validate-once-per-build-session
-
-// RUN: %clang -fno-modules -fmodules-validate-system-headers -### %s 2>&1 | 
FileCheck -check-prefix=VALIDATE_SYSTEM_FLAG %s
-// VALIDATE_SYSTEM_FLAG-NOT: -fmodules-validate-system-headers
+// RUN: %clang -fno-modules -fmodules-validate-once-per-build-session 
-fmodules-validate-system-headers -fmodule-map-file=module.modulemap \
+// RUN:   -### %s 2>&1 | FileCheck -check-prefix=IGNORED_FLAGS %s
+// IGNORED_FLAGS-NOT: -fmodules-validate-once-per-build-session
+// IGNORED_FLAGS-NOT: -fmodules-validate-system-headers
+// IGNORED_FLAGS-NOT: -fmodule-map-file
+
+// RUN: %clang -fno-modules -fmodules-decluse 
-fmodule-map-file=module.modulemap -### %s 2>&1 | FileCheck 
-check-prefix=MMAPFILE %s
+// RUN: %clang -fno-modules -fmodules-strict-decluse 
-fmodule-map-file=module.modulemap -### %s 2>&1 | FileCheck 
-check-prefix=MMAPFILE %s
+// RUN: %clang -fno-modules -Xclang -fmodules-local-submodule-visibility 
-fmodule-map-file=module.modulemap -### %s 2>&1 | FileCheck 
-check-prefix=MMAPFILE %s
+// MMAPFILE: -fmodule-map-file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3735,23 +3735,47 @@
   CmdArgs.push_back("-fvalidate-ast-input-files-content");
   }
 
-  // -fmodule-name specifies the module that is currently being built (or
-  // used for header checking by -fmodule-maps).
-  Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
-
-  // -fmodule-map-file can be used to specify files containing module
-  // definitions.
-  Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
-
-  // -fbuiltin-module-map can be used to load the clang
-  // builtin headers modulemap file.
-  if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
-SmallString<128> BuiltinModuleMap(D.ResourceDir);
-llvm::sys::path::append(BuiltinModuleMap, "include");
-llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
-if (llvm::sys::fs::exists(BuiltinModuleMap))
-  CmdArgs.push_back(
-  Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+  // Check whether any "-fmodule-map-file=" flags are relevant or not. This is
+  // useful for pruning the cc1 arguments and removing unnecessary input
+  // dependencies.
+  bool ModuleMapsAreRelevant = [&]() -> bool {
+if (HaveModules)
+  return true;
+if (Args.hasFlag(options::OPT_fmodules_decluse,
+ options::OPT_fno_modules_decluse, false) ||
+Args.hasFlag(options::OPT_fmodules_strict_decluse,
+ options::OPT_fno_modules_strict_decluse, false))
+  return true;
+for (auto Arg : Args.filtered(options::OPT_Xclang)) {
+  if (StringRef(Arg->getValue()) == "-fmodules-local-submodule-visibility")
+return true;
+}
+return false;
+  }();
+
+  if (ModuleMapsAreRelevant) {
+// -fmodule-name specifies the module that is currently being built (or
+// used for header checking by -fmodule-maps).
+Args.AddLastArg(CmdArgs, options::OPT_fmodule_name_EQ);
+
+// -fmodule-map-file can be used to specify files containing module
+// definitions.
+Args.AddAllArgs(CmdArgs, options::OPT_fmodule_map_file);
+
+// -fbuiltin-module-map can be used to load the clang
+// builtin headers modulemap file.
+if (Args.hasArg(options::OPT_fbuiltin_module_map)) {
+  SmallString<128> BuiltinModuleMap(D.ResourceDir);
+  llvm::sys::path::append(BuiltinModuleMap, "include");
+  llvm::sys::path::append(BuiltinModuleMap, "module.modulemap");
+  if (llvm::sys::fs::exists(BuiltinModuleMap))
+CmdArgs.push_back(
+Args.MakeArgString("-fmodule-map-file=" + BuiltinModuleMap));
+}
+  } else {
+Args.ClaimAllArgs(options::OPT_fmodule_name_EQ);
+Args.Clai

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D132801#3761253 , @akyrtzi wrote:

> In D132801#3760014 , @rsmith wrote:
>
>> This doesn't look right to me -- we still use module maps when modules are 
>> disabled to enforce layering checking, and when 
>> `-fmodules-local-submodule-visibility` is enabled but `-fmodules` is 
>> disabled we'll use them to provide modular semantics without pre-building 
>> modules. I'm going to revert.
>
> Sorry, I wasn't aware of that. Could `-fmodule-map-file=` be pruned out when 
> `-fmodules-local-submodule-visibility` or `-fmodules-decluse` are not enabled?

I've opened https://reviews.llvm.org/D133229 for your consideration, let me 
know whether this addresses your concerns 🙏


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132801/new/

https://reviews.llvm.org/D132801

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133229: [driver] Prune module-map related flags, if they are not going to be needed

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D133229#3768101 , @rsmith wrote:

> I think the approach you're taking here is probably doomed -- too many things 
> in Clang depend on whether we've read module map files, and it seems unlikely 
> to me that you'll be able to catch all of them from the driver.

I see, the driver approach does seem like a non-starter, thank you for the 
feedback! This needs reevaluation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133229/new/

https://reviews.llvm.org/D133229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a subscriber: mgorny.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Directive `dependency_directives_scan::tokens_present_before_eof` is introduced 
to indicate there were tokens present before
the last scanned dependency directive and EOF.
This is useful to ensure we correctly identify the macro guards when lexing 
using the dependency directives.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133357

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
  clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Index: clang/unittests/Lex/PPDependencyDirectivesTest.cpp
===
--- /dev/null
+++ clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -0,0 +1,150 @@
+//===- unittests/Lex/PPDependencyDirectivesTest.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+// The test fixture.
+class PPDependencyDirectivesTest : public ::testing::Test {
+protected:
+  PPDependencyDirectivesTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+TargetOpts->Triple = "x86_64-apple-macos12";
+Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr TargetOpts;
+  IntrusiveRefCntPtr Target;
+};
+
+class IncludeCollector : public PPCallbacks {
+public:
+  Preprocessor &PP;
+  SmallVectorImpl &IncludedFiles;
+
+  IncludeCollector(Preprocessor &PP, SmallVectorImpl &IncludedFiles)
+  : PP(PP), IncludedFiles(IncludedFiles) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != LexedFileChangeReason::EnterFile)
+  return;
+if (FID == PP.getPredefinesFileID())
+  return;
+StringRef Filename =
+PP.getSourceManager().getSLocEntry(FID).getFile().getName();
+IncludedFiles.push_back(Filename);
+  }
+};
+
+TEST_F(PPDependencyDirectivesTest, MacroGuard) {
+  // "head1.h" has a macro guard and should only be included once.
+  // "head2.h" and "head3.h" have tokens following the macro check, they should
+  // be included multiple times.
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->addFile(
+  "head1.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H1_H\n#define H1_H\n#endif\n"));
+  VFS->addFile(
+  "head2.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H2_H\n#define H2_H\n#endif\n\n"
+   "extern int foo;\n"));
+  VFS->addFile("head3.h", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#ifndef H3_H\n#define H3_H\n#endif\n\n"
+   "#ifdef SOMEMAC\nextern int foo;\n#endif\n"));
+  VFS->addFile("main.c", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"head1.h\"\n#include \"head1.h\"\n"
+   "#include \"head2.h\"\n#include \"head2.h\"\n"
+   "#include \"head3.h\"\n#include \"head3.h\"\n"));
+  FileMgr.setVirtualFileSystem(VFS);
+
+  Optional FE;
+  ASSERT_THAT_ERROR(FileMgr.getFileRef("main.c").moveInto(FE),
+llvm::Succeeded());
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(*FE, SourceLocation(), SrcMgr::C_User));
+
+  struct DepDirectives {
+SmallVector Tokens;
+SmallVector Directives;
+  };
+  SmallVector> DepDirectivesObjects;
+
+  auto getDependencyDirectives = [&](FileEntryRef File)
+

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 458173.
akyrtzi added a comment.

Remove a couple of unused local variables.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
  clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Index: clang/unittests/Lex/PPDependencyDirectivesTest.cpp
===
--- /dev/null
+++ clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -0,0 +1,148 @@
+//===- unittests/Lex/PPDependencyDirectivesTest.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+// The test fixture.
+class PPDependencyDirectivesTest : public ::testing::Test {
+protected:
+  PPDependencyDirectivesTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+TargetOpts->Triple = "x86_64-apple-macos12";
+Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr TargetOpts;
+  IntrusiveRefCntPtr Target;
+};
+
+class IncludeCollector : public PPCallbacks {
+public:
+  Preprocessor &PP;
+  SmallVectorImpl &IncludedFiles;
+
+  IncludeCollector(Preprocessor &PP, SmallVectorImpl &IncludedFiles)
+  : PP(PP), IncludedFiles(IncludedFiles) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != LexedFileChangeReason::EnterFile)
+  return;
+if (FID == PP.getPredefinesFileID())
+  return;
+StringRef Filename =
+PP.getSourceManager().getSLocEntry(FID).getFile().getName();
+IncludedFiles.push_back(Filename);
+  }
+};
+
+TEST_F(PPDependencyDirectivesTest, MacroGuard) {
+  // "head1.h" has a macro guard and should only be included once.
+  // "head2.h" and "head3.h" have tokens following the macro check, they should
+  // be included multiple times.
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->addFile(
+  "head1.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H1_H\n#define H1_H\n#endif\n"));
+  VFS->addFile(
+  "head2.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H2_H\n#define H2_H\n#endif\n\n"
+   "extern int foo;\n"));
+  VFS->addFile("head3.h", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#ifndef H3_H\n#define H3_H\n#endif\n\n"
+   "#ifdef SOMEMAC\nextern int foo;\n#endif\n"));
+  VFS->addFile("main.c", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"head1.h\"\n#include \"head1.h\"\n"
+   "#include \"head2.h\"\n#include \"head2.h\"\n"
+   "#include \"head3.h\"\n#include \"head3.h\"\n"));
+  FileMgr.setVirtualFileSystem(VFS);
+
+  Optional FE;
+  ASSERT_THAT_ERROR(FileMgr.getFileRef("main.c").moveInto(FE),
+llvm::Succeeded());
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(*FE, SourceLocation(), SrcMgr::C_User));
+
+  struct DepDirectives {
+SmallVector Tokens;
+SmallVector Directives;
+  };
+  SmallVector> DepDirectivesObjects;
+
+  auto getDependencyDirectives = [&](FileEntryRef File)
+  -> Optional> {
+DepDirectivesObjects.push_back(std::make_unique());
+StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
+bool Err = scanSourceForDependencyDirectives(
+Input, DepDirectivesObjects.back()->Tokens,
+DepDirectivesObjects.back()->Directives);
+EX

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///

benlangmuir wrote:
> Why would you want to print without this? It seems important for correctness 
> of the output.  I would have expected we would always print it.
The `-print-dependency-directives-minimized-source` clang option is using this 
function, and if you print the sources with "" at the end then the 
source text will not be parsable.

But the tests that pass `-print-dependency-directives-minimized-source` don't 
try to parse the code, so I think I can switch the default to be `true` for 
printing the "tokens-before-eof marker" and if a caller has a need to ignore it 
it can pass `false` for the parameter.



Comment at: clang/lib/Lex/DependencyDirectivesScanner.cpp:868
+PrintMarkerForTokensBeforeEOF)
+  OS << "";
 Optional PrevTokenKind;

benlangmuir wrote:
> How about "TokBeforeEOF"?  I think "BEOF" is too cryptic.
SGTM 👍


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/include/clang/Lex/DependencyDirectivesScanner.h:131
+/// \p dependency_directives_scan::tokens_present_before_eof, otherwise this
+/// directive will be ignored.
 ///

benlangmuir wrote:
> benlangmuir wrote:
> > akyrtzi wrote:
> > > benlangmuir wrote:
> > > > Why would you want to print without this? It seems important for 
> > > > correctness of the output.  I would have expected we would always print 
> > > > it.
> > > The `-print-dependency-directives-minimized-source` clang option is using 
> > > this function, and if you print the sources with "" at the end 
> > > then the source text will not be parsable.
> > > 
> > > But the tests that pass `-print-dependency-directives-minimized-source` 
> > > don't try to parse the code, so I think I can switch the default to be 
> > > `true` for printing the "tokens-before-eof marker" and if a caller has a 
> > > need to ignore it it can pass `false` for the parameter.
> > If someone uses `-print-dependency-directives-minimized-source` and creates 
> > a minimized file for each header, it will "parse", but it won't behave 
> > correctly for multiple includes, right?  My preference is we don't allow 
> > printing this without the TokBEOF.  If we care about making it parse, we 
> > should add a real token of some kind -- maybe there is a no-op `#pragma` or 
> > something?
> Oops, missed half my comment. Meant to also add:
> 
> > But the tests that pass -print-dependency-directives-minimized-source don't 
> > try to parse the code, so I think I can switch the default to be true for 
> > printing the "tokens-before-eof marker" and if a caller has a need to 
> > ignore it it can pass false for the parameter.
> 
> SGTM
Ok, you convinced me that we should just always print it. This function is for 
testing purposes only anyway, if later on we actually have a need to parse back 
the minimized source we can re-evaluate what to do with that directive marker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 458360.
akyrtzi added a comment.

Always print `tokens_present_before_eof` and print it as "" for 
clarity.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
  clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Index: clang/unittests/Lex/PPDependencyDirectivesTest.cpp
===
--- /dev/null
+++ clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -0,0 +1,148 @@
+//===- unittests/Lex/PPDependencyDirectivesTest.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+// The test fixture.
+class PPDependencyDirectivesTest : public ::testing::Test {
+protected:
+  PPDependencyDirectivesTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+TargetOpts->Triple = "x86_64-apple-macos12";
+Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr TargetOpts;
+  IntrusiveRefCntPtr Target;
+};
+
+class IncludeCollector : public PPCallbacks {
+public:
+  Preprocessor &PP;
+  SmallVectorImpl &IncludedFiles;
+
+  IncludeCollector(Preprocessor &PP, SmallVectorImpl &IncludedFiles)
+  : PP(PP), IncludedFiles(IncludedFiles) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != LexedFileChangeReason::EnterFile)
+  return;
+if (FID == PP.getPredefinesFileID())
+  return;
+StringRef Filename =
+PP.getSourceManager().getSLocEntry(FID).getFile().getName();
+IncludedFiles.push_back(Filename);
+  }
+};
+
+TEST_F(PPDependencyDirectivesTest, MacroGuard) {
+  // "head1.h" has a macro guard and should only be included once.
+  // "head2.h" and "head3.h" have tokens following the macro check, they should
+  // be included multiple times.
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->addFile(
+  "head1.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H1_H\n#define H1_H\n#endif\n"));
+  VFS->addFile(
+  "head2.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H2_H\n#define H2_H\n#endif\n\n"
+   "extern int foo;\n"));
+  VFS->addFile("head3.h", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#ifndef H3_H\n#define H3_H\n#endif\n\n"
+   "#ifdef SOMEMAC\nextern int foo;\n#endif\n"));
+  VFS->addFile("main.c", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"head1.h\"\n#include \"head1.h\"\n"
+   "#include \"head2.h\"\n#include \"head2.h\"\n"
+   "#include \"head3.h\"\n#include \"head3.h\"\n"));
+  FileMgr.setVirtualFileSystem(VFS);
+
+  Optional FE;
+  ASSERT_THAT_ERROR(FileMgr.getFileRef("main.c").moveInto(FE),
+llvm::Succeeded());
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(*FE, SourceLocation(), SrcMgr::C_User));
+
+  struct DepDirectives {
+SmallVector Tokens;
+SmallVector Directives;
+  };
+  SmallVector> DepDirectivesObjects;
+
+  auto getDependencyDirectives = [&](FileEntryRef File)
+  -> Optional> {
+DepDirectivesObjects.push_back(std::make_unique());
+StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
+bool Err = scanSourceForDependencyDirectives(
+Input, DepDirectivesObjects.back()->Tokens,
+DepDirectivesObjec

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi marked 4 inline comments as done.
akyrtzi added a comment.

@benlangmuir see latest diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 458499.
akyrtzi added a comment.

Remove leftover doc-comment parameter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
  clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Index: clang/unittests/Lex/PPDependencyDirectivesTest.cpp
===
--- /dev/null
+++ clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -0,0 +1,148 @@
+//===- unittests/Lex/PPDependencyDirectivesTest.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+// The test fixture.
+class PPDependencyDirectivesTest : public ::testing::Test {
+protected:
+  PPDependencyDirectivesTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+TargetOpts->Triple = "x86_64-apple-macos12";
+Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr TargetOpts;
+  IntrusiveRefCntPtr Target;
+};
+
+class IncludeCollector : public PPCallbacks {
+public:
+  Preprocessor &PP;
+  SmallVectorImpl &IncludedFiles;
+
+  IncludeCollector(Preprocessor &PP, SmallVectorImpl &IncludedFiles)
+  : PP(PP), IncludedFiles(IncludedFiles) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != LexedFileChangeReason::EnterFile)
+  return;
+if (FID == PP.getPredefinesFileID())
+  return;
+StringRef Filename =
+PP.getSourceManager().getSLocEntry(FID).getFile().getName();
+IncludedFiles.push_back(Filename);
+  }
+};
+
+TEST_F(PPDependencyDirectivesTest, MacroGuard) {
+  // "head1.h" has a macro guard and should only be included once.
+  // "head2.h" and "head3.h" have tokens following the macro check, they should
+  // be included multiple times.
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->addFile(
+  "head1.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H1_H\n#define H1_H\n#endif\n"));
+  VFS->addFile(
+  "head2.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H2_H\n#define H2_H\n#endif\n\n"
+   "extern int foo;\n"));
+  VFS->addFile("head3.h", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#ifndef H3_H\n#define H3_H\n#endif\n\n"
+   "#ifdef SOMEMAC\nextern int foo;\n#endif\n"));
+  VFS->addFile("main.c", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"head1.h\"\n#include \"head1.h\"\n"
+   "#include \"head2.h\"\n#include \"head2.h\"\n"
+   "#include \"head3.h\"\n#include \"head3.h\"\n"));
+  FileMgr.setVirtualFileSystem(VFS);
+
+  Optional FE;
+  ASSERT_THAT_ERROR(FileMgr.getFileRef("main.c").moveInto(FE),
+llvm::Succeeded());
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(*FE, SourceLocation(), SrcMgr::C_User));
+
+  struct DepDirectives {
+SmallVector Tokens;
+SmallVector Directives;
+  };
+  SmallVector> DepDirectivesObjects;
+
+  auto getDependencyDirectives = [&](FileEntryRef File)
+  -> Optional> {
+DepDirectivesObjects.push_back(std::make_unique());
+StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
+bool Err = scanSourceForDependencyDirectives(
+Input, DepDirectivesObjects.back()->Tokens,
+DepDirectivesObjects.back()->Directives);
+EXPECT

[PATCH] D133357: [Lex/DependencyDirectivesScanner] Keep track of the presence of tokens between the last scanned directive and EOF

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaa484c90cf59: [Lex/DependencyDirectivesScanner] Keep track 
of the presence of tokens between… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133357/new/

https://reviews.llvm.org/D133357

Files:
  clang/include/clang/Lex/DependencyDirectivesScanner.h
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/lib/Lex/Lexer.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
  clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Index: clang/unittests/Lex/PPDependencyDirectivesTest.cpp
===
--- /dev/null
+++ clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -0,0 +1,148 @@
+//===- unittests/Lex/PPDependencyDirectivesTest.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Lex/DependencyDirectivesScanner.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+// The test fixture.
+class PPDependencyDirectivesTest : public ::testing::Test {
+protected:
+  PPDependencyDirectivesTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+TargetOpts->Triple = "x86_64-apple-macos12";
+Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr TargetOpts;
+  IntrusiveRefCntPtr Target;
+};
+
+class IncludeCollector : public PPCallbacks {
+public:
+  Preprocessor &PP;
+  SmallVectorImpl &IncludedFiles;
+
+  IncludeCollector(Preprocessor &PP, SmallVectorImpl &IncludedFiles)
+  : PP(PP), IncludedFiles(IncludedFiles) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != LexedFileChangeReason::EnterFile)
+  return;
+if (FID == PP.getPredefinesFileID())
+  return;
+StringRef Filename =
+PP.getSourceManager().getSLocEntry(FID).getFile().getName();
+IncludedFiles.push_back(Filename);
+  }
+};
+
+TEST_F(PPDependencyDirectivesTest, MacroGuard) {
+  // "head1.h" has a macro guard and should only be included once.
+  // "head2.h" and "head3.h" have tokens following the macro check, they should
+  // be included multiple times.
+
+  auto VFS = new llvm::vfs::InMemoryFileSystem();
+  VFS->addFile(
+  "head1.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H1_H\n#define H1_H\n#endif\n"));
+  VFS->addFile(
+  "head2.h", 0,
+  llvm::MemoryBuffer::getMemBuffer("#ifndef H2_H\n#define H2_H\n#endif\n\n"
+   "extern int foo;\n"));
+  VFS->addFile("head3.h", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#ifndef H3_H\n#define H3_H\n#endif\n\n"
+   "#ifdef SOMEMAC\nextern int foo;\n#endif\n"));
+  VFS->addFile("main.c", 0,
+   llvm::MemoryBuffer::getMemBuffer(
+   "#include \"head1.h\"\n#include \"head1.h\"\n"
+   "#include \"head2.h\"\n#include \"head2.h\"\n"
+   "#include \"head3.h\"\n#include \"head3.h\"\n"));
+  FileMgr.setVirtualFileSystem(VFS);
+
+  Optional FE;
+  ASSERT_THAT_ERROR(FileMgr.getFileRef("main.c").moveInto(FE),
+llvm::Succeeded());
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(*FE, SourceLocation(), SrcMgr::C_User));
+
+  struct DepDirectives {
+SmallVector Tokens;
+SmallVector Directives;
+  };
+  SmallVector> DepDirectivesObjects;
+
+  auto getDependencyDirectives = [&](FileEntryRef File)
+  -> Optional> {
+DepDirectivesObjects.push_back(std::make_unique());
+StringRef Input = (*FileMgr.getBufferForFile(File))->getBuffer();
+bool E

[PATCH] D133674: [Lex/DependencyDirectivesScanner] Handle the case where the source line starts with a `tok::hashhash`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133674

Files:
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,11 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,11 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133674: [Lex/DependencyDirectivesScanner] Handle the case where the source line starts with a `tok::hashhash`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D133674#3784602 , @jansvoboda11 
wrote:

> Could you explain why this is necessary and even correct? I'd expect Clang to 
> give an error when seeing `##` in this position, and I'd expect the scanner 
> to do the same.

`##` is lexed as `tok::hashhash`; it is ignored by the preprocessor (it is not 
treated as the start of a preprocessor directive) and passed on to the parser 
to interpret, like any other token.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133674/new/

https://reviews.llvm.org/D133674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133674: [Lex/DependencyDirectivesScanner] Handle the case where the source line starts with a `tok::hashhash`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 459888.
akyrtzi added a comment.

Add comment to clarify why we skip if `tok::hashhash` is encountered.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133674/new/

https://reviews.llvm.org/D133674

Files:
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,14 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+// A \p tok::hashhash at this location is passed by the preprocessor to the
+// parser to interpret, like any other token. So for dependency scanning
+// skip it like a normal token not affecting the preprocessor.
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,14 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+// A \p tok::hashhash at this location is passed by the preprocessor to the
+// parser to interpret, like any other token. So for dependency scanning
+// skip it like a normal token not affecting the preprocessor.
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133674: [Lex/DependencyDirectivesScanner] Handle the case where the source line starts with a `tok::hashhash`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D133674#3784710 , @akyrtzi wrote:

> In D133674#3784602 , @jansvoboda11 
> wrote:
>
>> Could you explain why this is necessary and even correct? I'd expect Clang 
>> to give an error when seeing `##` in this position, and I'd expect the 
>> scanner to do the same.
>
> `##` is lexed as `tok::hashhash`; it is ignored by the preprocessor (it is 
> not treated as the start of a preprocessor directive) and passed on to the 
> parser to interpret, like any other token.

@jansvoboda11 I've added a comment in code to make it clear why it skips.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133674/new/

https://reviews.llvm.org/D133674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133674: [Lex/DependencyDirectivesScanner] Handle the case where the source line starts with a `tok::hashhash`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb340c5ae4221: [Lex/DependencyDirectivesScanner] Handle the 
case where the source line starts… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133674/new/

https://reviews.llvm.org/D133674

Files:
  clang/lib/Lex/DependencyDirectivesScanner.cpp
  clang/unittests/Lex/DependencyDirectivesScannerTest.cpp


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,14 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+// A \p tok::hashhash at this location is passed by the preprocessor to the
+// parser to interpret, like any other token. So for dependency scanning
+// skip it like a normal token not affecting the preprocessor.
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 


Index: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
===
--- clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -124,6 +124,21 @@
   EXPECT_STREQ("#define MACRO a\n", Out.data());
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest, HashHash) {
+  SmallVector Out;
+
+  StringRef Source = R"(
+#define S
+#if 0
+  ##pragma cool
+  ##include "t.h"
+#endif
+#define E
+)";
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(Source, Out));
+  EXPECT_STREQ("#define S\n#if 0\n#endif\n#define E\n", Out.data());
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
   SmallVector Out;
   SmallVector Tokens;
Index: clang/lib/Lex/DependencyDirectivesScanner.cpp
===
--- clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -742,6 +742,14 @@
 
   // Lex '#'.
   const dependency_directives_scan::Token &HashTok = lexToken(First, End);
+  if (HashTok.is(tok::hashhash)) {
+// A \p tok::hashhash at this location is passed by the preprocessor to the
+// parser to interpret, like any other token. So for dependency scanning
+// skip it like a normal token not affecting the preprocessor.
+skipLine(First, End);
+assert(First <= End);
+return false;
+  }
   assert(HashTok.is(tok::hash));
   (void)HashTok;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128772: [Lex] Make sure to notify `MultipleIncludeOpt` for "read tokens" during fast dependency directive lexing

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Otherwise a header may be erroneously marked as having a header macro guard and 
won't get re-included.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128772

Files:
  clang/lib/Lex/Lexer.cpp
  clang/test/ClangScanDeps/more-content-after-headerguard.c


Index: clang/test/ClangScanDeps/more-content-after-headerguard.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/more-content-after-headerguard.c
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// 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 | FileCheck %s
+
+// CHECK: t.c
+// CHECK: top.h
+// CHECK: n1.h
+// CHECK: n2.h
+// CHECK: n3.h
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/t.c",
+"file": "DIR/t.c"
+  }
+]
+
+//--- t.c
+
+#include "top.h"
+#define INCLUDE_N3
+#include "top.h"
+
+//--- top.h
+#ifndef _TOP_H_
+#define _TOP_H_
+
+#include "n1.h"
+
+#endif
+
+// More stuff after following '#endif', should invalidate the macro guard 
optimization.
+// and allow `top.h` to get re-included.
+#include "n2.h"
+
+//--- n1.h
+
+//--- n2.h
+#ifdef INCLUDE_N3
+#include "n3.h"
+#endif
+
+//--- n3.h
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4248,6 +4248,10 @@
 
   const dependency_directives_scan::Token &DDTok =
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.
+MIOpt.ReadToken();
+  }
 
   const char *TokPtr = convertDependencyDirectiveToken(DDTok, Result);
 


Index: clang/test/ClangScanDeps/more-content-after-headerguard.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/more-content-after-headerguard.c
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// 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 | FileCheck %s
+
+// CHECK: t.c
+// CHECK: top.h
+// CHECK: n1.h
+// CHECK: n2.h
+// CHECK: n3.h
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/t.c",
+"file": "DIR/t.c"
+  }
+]
+
+//--- t.c
+
+#include "top.h"
+#define INCLUDE_N3
+#include "top.h"
+
+//--- top.h
+#ifndef _TOP_H_
+#define _TOP_H_
+
+#include "n1.h"
+
+#endif
+
+// More stuff after following '#endif', should invalidate the macro guard optimization.
+// and allow `top.h` to get re-included.
+#include "n2.h"
+
+//--- n1.h
+
+//--- n2.h
+#ifdef INCLUDE_N3
+#include "n3.h"
+#endif
+
+//--- n3.h
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4248,6 +4248,10 @@
 
   const dependency_directives_scan::Token &DDTok =
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.
+MIOpt.ReadToken();
+  }
 
   const char *TokPtr = convertDependencyDirectiveToken(DDTok, Result);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128772: [Lex] Make sure to notify `MultipleIncludeOpt` for "read tokens" during fast dependency directive lexing

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/Lex/Lexer.cpp:4251
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.

benlangmuir wrote:
> Why do we need the >1 check? I'm not familiar with the details of MIO here.
I'm making sure that `MIOpt.ReadToken()` is called for every token except the 
starting hash of a preprocessor directive, to match what regular lexing is 
doing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128772/new/

https://reviews.llvm.org/D128772

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128772: [Lex] Make sure to notify `MultipleIncludeOpt` for "read tokens" during fast dependency directive lexing

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc68b8c84eb17: [Lex] Make sure to notify `MultipleIncludeOpt` 
for "read tokens" during fast… (authored by akyrtzi).

Changed prior to commit:
  https://reviews.llvm.org/D128772?vs=440813&id=441191#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128772/new/

https://reviews.llvm.org/D128772

Files:
  clang/lib/Lex/Lexer.cpp
  clang/test/ClangScanDeps/more-content-after-headerguard.c


Index: clang/test/ClangScanDeps/more-content-after-headerguard.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/more-content-after-headerguard.c
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// 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 | FileCheck %s
+
+// CHECK: t.c
+// CHECK: top.h
+// CHECK: n1.h
+// CHECK: n2.h
+// CHECK: n3.h
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/t.c",
+"file": "DIR/t.c"
+  }
+]
+
+//--- t.c
+
+#include "top.h"
+#define INCLUDE_N3
+#include "top.h"
+
+//--- top.h
+#ifndef _TOP_H_
+#define _TOP_H_
+
+#include "n1.h"
+
+#endif
+
+// More stuff after following '#endif', should invalidate the macro guard 
optimization,
+// and allow `top.h` to get re-included.
+#include "n2.h"
+
+//--- n1.h
+
+//--- n2.h
+#ifdef INCLUDE_N3
+#include "n3.h"
+#endif
+
+//--- n3.h
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4248,6 +4248,10 @@
 
   const dependency_directives_scan::Token &DDTok =
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.
+MIOpt.ReadToken();
+  }
 
   const char *TokPtr = convertDependencyDirectiveToken(DDTok, Result);
 


Index: clang/test/ClangScanDeps/more-content-after-headerguard.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/more-content-after-headerguard.c
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// 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 | FileCheck %s
+
+// CHECK: t.c
+// CHECK: top.h
+// CHECK: n1.h
+// CHECK: n2.h
+// CHECK: n3.h
+
+//--- cdb.json.template
+[
+  {
+"directory": "DIR",
+"command": "clang -fsyntax-only DIR/t.c",
+"file": "DIR/t.c"
+  }
+]
+
+//--- t.c
+
+#include "top.h"
+#define INCLUDE_N3
+#include "top.h"
+
+//--- top.h
+#ifndef _TOP_H_
+#define _TOP_H_
+
+#include "n1.h"
+
+#endif
+
+// More stuff after following '#endif', should invalidate the macro guard optimization,
+// and allow `top.h` to get re-included.
+#include "n2.h"
+
+//--- n1.h
+
+//--- n2.h
+#ifdef INCLUDE_N3
+#include "n3.h"
+#endif
+
+//--- n3.h
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -4248,6 +4248,10 @@
 
   const dependency_directives_scan::Token &DDTok =
   DepDirectives.front().Tokens[NextDepDirectiveTokenIndex++];
+  if (NextDepDirectiveTokenIndex > 1 || DDTok.Kind != tok::hash) {
+// Read something other than a preprocessor directive hash.
+MIOpt.ReadToken();
+  }
 
   const char *TokPtr = convertDependencyDirectiveToken(DDTok, Result);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added subscribers: shchenz, kbarton, nemanjai.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a preprocessor callback focused on the lexed file changing, without 
conflating effects of line number directives and other pragmas.
A client that only cares about what files the lexer processes, like dependency 
generation, can use this more straightforward
callback instead of `PPCallbacks::FileChanged()`. Clients that want the pragma 
directive effects as well can keep using `FileChanged()`.

A use case where `PPCallbacks::LexedFileChanged()` is particularly simpler to 
use than `FileChanged()` is in a situation
where a client wants to keep track of lexed file changes that include changes 
from/to the predefines buffer, where it becomes
unnecessary complicated trying to use `FileChanged()` while filtering out the 
pragma directives effects callbacks.

For `LexedFileChanged()` take the opportunity to also provide information about 
the prior `FileID` the `Lexer` moved from, even when
entering a new file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128947

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -132,6 +133,15 @@
 
 Callbacks->FileChanged(CurLexer->getFileLoc(),
PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,22 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
+  return;
+if (PP.getPredefinesFileID() == FID)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename = PP.getSourceManager().getFilename(FID))
  

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:1016
+return *Name;
   return StringRef();
 }

benlangmuir wrote:
> Just a suggestion: `value_or` can be a nice way to express simple cases like 
> this:
> 
> ```
> getFilename(getFileID(SpellingLoc)).value_or(StringRef());
> ```
Thank you for the tip!



Comment at: clang/lib/Lex/PPLexerChange.cpp:136
PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;

benlangmuir wrote:
> Why does `LexedFileChanged` have `PrevFID` set, but `FileChanged` does not 
> (it has a default argument of `FileID()`?  I would have expected that when 
> you call both `FileChanged` and `LexedFileChanged` for the same event this 
> would match.
I didn't want to change the "contract" of `FileChanged()` as part of these 
changes but it's probably unlikely that someone depends on `FileID` being 
invalid, I'll give it a try.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 441551.
akyrtzi added a comment.

Use `Optional::value_or()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -132,6 +133,15 @@
 
 Callbacks->FileChanged(CurLexer->getFileLoc(),
PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,22 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
+  return;
+if (PP.getPredefinesFileID() == FID)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename = PP.getSourceManager().getFilename(FID))
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
@@ -90,7 +89,9 @@
 /*IsMissing=*/false);
   }
 
-  void EndOfMainFile() override { DepCollector.finishedMainFile(Diags); }
+  void EndOfMainFile() override {
+DepCollector.finishedMainFile(PP.getDiagnostics());
+  }
 };
 
 struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
@@ -175,8 +176,7 @@
 
 DependencyCollector::~DependencyCollector() { }
 void DependencyCollector::attachToPreprocessor(Preprocessor &PP) {
-  PP.addPPCallbacks(std::make_unique(
-  *this, PP.getSourceManager(), PP.getDiagnostics()));
+  PP.addPPCallbacks(std::make_unique(*this, PP));
   PP.getHeaderSearchInfo().getModuleMap().addModuleMapCallbacks(
   std::make_unique(*this));
 }
Index: clang/lib/Basic/SourceManager.cpp
===

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 441556.
akyrtzi added a comment.

Pass a value for `PrevFID` for `FileChanged()` callback as well, for 
`PPCallbacks::EnterFile` reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -130,8 +131,17 @@
 SrcMgr::CharacteristicKind FileType =
SourceMgr.getFileCharacteristic(CurLexer->getFileLoc());
 
-Callbacks->FileChanged(CurLexer->getFileLoc(),
-   PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->FileChanged(CurLexer->getFileLoc(), PPCallbacks::EnterFile,
+   FileType, PrevFID);
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,22 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
+  return;
+if (PP.getPredefinesFileID() == FID)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename = PP.getSourceManager().getFilename(FID))
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
@@ -90,7 +89,9 @@
 /*IsMissing=*/false);
   }
 
-  void EndOfMainFile() override { DepCollector.finishedMainFile(Diags); }
+  void EndOfMainFile() override {
+DepCollector.finishedMainFile(PP.getDiagnostics());
+  }
 };
 
 struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
@@ -175,8 +176,7 @@
 
 DependencyCollector::~DependencyCollector() { }
 void DependencyCollector::attachToPreprocessor(Preprocessor &PP) {
-  PP.addPPCallbacks(std::make_unique(
-

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 441587.
akyrtzi added a comment.
Herald added a project: clang-tools-extra.

Update `clang-tools-extra/test/pp-trace/pp-trace-include.cpp` to accomodate for 
`PrevFID` getting a value and
preserve using `getFileEntryForID()` for the `SourceManager::getFilename()` 
implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

Files:
  clang-tools-extra/test/pp-trace/pp-trace-include.cpp
  clang/include/clang/Basic/SourceManager.h
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -130,8 +131,17 @@
 SrcMgr::CharacteristicKind FileType =
SourceMgr.getFileCharacteristic(CurLexer->getFileLoc());
 
-Callbacks->FileChanged(CurLexer->getFileLoc(),
-   PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->FileChanged(CurLexer->getFileLoc(), PPCallbacks::EnterFile,
+   FileType, PrevFID);
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,22 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
+  return;
+if (PP.getPredefinesFileID() == FID)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename = PP.getSourceManager().getFilename(FID))
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
@@ -90,7 +89,9 @@
 /*IsMissing=*/false);
   }
 
-  void EndOfMainFile() override { DepCollector.finishedMainFile(Diags); }
+  void EndOfMainFile() override {
+DepCollector.finishedMainFile(PP.getDiagnostics());
+  }
 };
 
 struct DepCollectorMMCallbacks : public M

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 441700.
akyrtzi added a comment.

Avoid changing the `SourceManager::getFilename()` implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

Files:
  clang-tools-extra/test/pp-trace/pp-trace-include.cpp
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -130,8 +131,17 @@
 SrcMgr::CharacteristicKind FileType =
SourceMgr.getFileCharacteristic(CurLexer->getFileLoc());
 
-Callbacks->FileChanged(CurLexer->getFileLoc(),
-   PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->FileChanged(CurLexer->getFileLoc(), PPCallbacks::EnterFile,
+   FileType, PrevFID);
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,21 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename =
+PP.getSourceManager().getNonBuiltinFilenameForID(FID))
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
@@ -90,7 +88,9 @@
 /*IsMissing=*/false);
   }
 
-  void EndOfMainFile() override { DepCollector.finishedMainFile(Diags); }
+  void EndOfMainFile() override {
+DepCollector.finishedMainFile(PP.getDiagnostics());
+  }
 };
 
 struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
@@ -175,8 +175,7 @@
 
 DependencyCollector::~DependencyCollector() { }
 void DependencyCollector::attachToPreprocessor(Preprocessor &PP) {
-  PP.addPPCallbacks(std::make_unique(
-  *this, PP.getSourceManager(), PP.getDiagnostics()));
+  PP.addPPCallbacks(std::make_u

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/Lex/PPLexerChange.cpp:136
PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;

akyrtzi wrote:
> benlangmuir wrote:
> > Why does `LexedFileChanged` have `PrevFID` set, but `FileChanged` does not 
> > (it has a default argument of `FileID()`?  I would have expected that when 
> > you call both `FileChanged` and `LexedFileChanged` for the same event this 
> > would match.
> I didn't want to change the "contract" of `FileChanged()` as part of these 
> changes but it's probably unlikely that someone depends on `FileID` being 
> invalid, I'll give it a try.
I'm passing a `PrevFID` value for `FileChanged()` as well and I only had to 
update one test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128947: [Lex] Introduce `PPCallbacks::LexedFileChanged()` preprocessor callback

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0d3a2b4c6601: [Lex] Introduce 
`PPCallbacks::LexedFileChanged()` preprocessor callback (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128947/new/

https://reviews.llvm.org/D128947

Files:
  clang-tools-extra/test/pp-trace/pp-trace-include.cpp
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPLexerChange.cpp

Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -111,6 +111,7 @@
 ///  and start lexing tokens from it instead of the current buffer.
 void Preprocessor::EnterSourceFileWithLexer(Lexer *TheLexer,
 ConstSearchDirIterator CurDir) {
+  PreprocessorLexer *PrevPPLexer = CurPPLexer;
 
   // Add the current lexer to the include stack.
   if (CurPPLexer || CurTokenLexer)
@@ -130,8 +131,17 @@
 SrcMgr::CharacteristicKind FileType =
SourceMgr.getFileCharacteristic(CurLexer->getFileLoc());
 
-Callbacks->FileChanged(CurLexer->getFileLoc(),
-   PPCallbacks::EnterFile, FileType);
+FileID PrevFID;
+SourceLocation EnterLoc;
+if (PrevPPLexer) {
+  PrevFID = PrevPPLexer->getFileID();
+  EnterLoc = PrevPPLexer->getSourceLocation();
+}
+Callbacks->FileChanged(CurLexer->getFileLoc(), PPCallbacks::EnterFile,
+   FileType, PrevFID);
+Callbacks->LexedFileChanged(CurLexer->getFileID(),
+PPCallbacks::LexedFileChangeReason::EnterFile,
+FileType, PrevFID, EnterLoc);
   }
 }
 
@@ -486,10 +496,13 @@
 
 // Notify the client, if desired, that we are in a new source file.
 if (Callbacks && !isEndOfMacro && CurPPLexer) {
+  SourceLocation Loc = CurPPLexer->getSourceLocation();
   SrcMgr::CharacteristicKind FileType =
-SourceMgr.getFileCharacteristic(CurPPLexer->getSourceLocation());
-  Callbacks->FileChanged(CurPPLexer->getSourceLocation(),
- PPCallbacks::ExitFile, FileType, ExitedFID);
+  SourceMgr.getFileCharacteristic(Loc);
+  Callbacks->FileChanged(Loc, PPCallbacks::ExitFile, FileType, ExitedFID);
+  Callbacks->LexedFileChanged(CurPPLexer->getFileID(),
+  PPCallbacks::LexedFileChangeReason::ExitFile,
+  FileType, ExitedFID, Loc);
 }
 
 // Restore conditional stack as well as the recorded
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -31,23 +31,21 @@
 namespace {
 struct DepCollectorPPCallbacks : public PPCallbacks {
   DependencyCollector &DepCollector;
-  SourceManager &SM;
-  DiagnosticsEngine &Diags;
-  DepCollectorPPCallbacks(DependencyCollector &L, SourceManager &SM,
-  DiagnosticsEngine &Diags)
-  : DepCollector(L), SM(SM), Diags(Diags) {}
-
-  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
-   SrcMgr::CharacteristicKind FileType,
-   FileID PrevFID) override {
-if (Reason != PPCallbacks::EnterFile)
+  Preprocessor &PP;
+  DepCollectorPPCallbacks(DependencyCollector &L, Preprocessor &PP)
+  : DepCollector(L), PP(PP) {}
+
+  void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
+SrcMgr::CharacteristicKind FileType, FileID PrevFID,
+SourceLocation Loc) override {
+if (Reason != PPCallbacks::LexedFileChangeReason::EnterFile)
   return;
 
 // Dependency generation really does want to go all the way to the
 // file entry for a source location to find out what is depended on.
 // We do not want #line markers to affect dependency generation!
-if (Optional Filename = SM.getNonBuiltinFilenameForID(
-SM.getFileID(SM.getExpansionLoc(Loc
+if (Optional Filename =
+PP.getSourceManager().getNonBuiltinFilenameForID(FID))
   DepCollector.maybeAddDependency(
   llvm::sys::path::remove_leading_dotslash(*Filename),
   /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
@@ -90,7 +88,9 @@
 /*IsMissing=*/false);
   }
 
-  void EndOfMainFile() override { DepCollector.finishedMainFile(Diags); }
+  void EndOfMainFile() override {
+DepCollector.finishedMainFile(PP.getDiagnostics());
+  }
 };
 
 struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
@@ -175,8 +175,7 @@
 
 DependencyCollector::~DependencyCollector() { }
 void DependencyCollector::attachToPreprocessor(Preprocessor &PP) {
-  PP.addPPCallbacks(std::make_unique(
-  *this, PP.getSourceMa

[PATCH] D129030: [Driver] Ignore the clang modules validation-related flags if clang modules are not enabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

If clang modules are not enabled it becomes unnecessary to read the session 
timestamp file in order
to pass `-fbuild-session-timestamp` to the `cc1` invocation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129030

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m

Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -4,27 +4,27 @@
 // RUN: %clang -fmodules -fno-modules -fmodules -### %s 2>&1 | FileCheck -check-prefix=CHECK-HAS-MODULES %s
 // CHECK-HAS-MODULES: -fmodules
 
-// RUN: %clang -fbuild-session-file=doesntexist -### %s 2>&1 | FileCheck -check-prefix=NOFILE %s
+// RUN: %clang -fmodules -fbuild-session-file=doesntexist -### %s 2>&1 | FileCheck -check-prefix=NOFILE %s
 // NOFILE: no such file or directory: 'doesntexist'
 
 // RUN: touch -m -a -t 201008011501 %t.build-session-file
-// RUN: %clang -fbuild-session-file=%t.build-session-file -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
 
-// RUN: %clang -fbuild-session-timestamp=1280703457 -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
+// RUN: %clang -fmodules -fbuild-session-timestamp=1280703457 -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
 // TIMESTAMP_ONLY: -fbuild-session-timestamp=128{{([[:digit:]]{7})[^[:digit:]]}}
 
-// RUN: %clang -fbuild-session-file=%t.build-session-file -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=CONFLICT %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=CONFLICT %s
 // CONFLICT: error: invalid argument '-fbuild-session-file={{.*}}.build-session-file' not allowed with '-fbuild-session-timestamp'
 
-// RUN: %clang -fbuild-session-timestamp=123 -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE %s
+// RUN: %clang -fmodules -fbuild-session-timestamp=123 -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE %s
 // MODULES_VALIDATE_ONCE: -fbuild-session-timestamp=123
 // MODULES_VALIDATE_ONCE: -fmodules-validate-once-per-build-session
 
-// RUN: %clang -fbuild-session-file=%t.build-session-file -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_FILE %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_FILE %s
 // MODULES_VALIDATE_ONCE_FILE: -fbuild-session-timestamp=128{{([[:digit:]]{7})[^[:digit:]]}}
 // MODULES_VALIDATE_ONCE_FILE: -fmodules-validate-once-per-build-session
 
-// RUN: %clang -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_ERR %s
+// RUN: %clang -fmodules -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_ERR %s
 // MODULES_VALIDATE_ONCE_ERR: option '-fmodules-validate-once-per-build-session' requires '-fbuild-session-timestamp=' or '-fbuild-session-file='
 
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT %s
@@ -33,16 +33,16 @@
 // RUN: %clang -fmodules -fsyntax-only -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT_MOD %s
 // MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT_MOD: -fmodules-validate-system-headers
 
-// RUN: %clang -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS %s
+// RUN: %clang -fmodules -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS %s
 // MODULES_VALIDATE_SYSTEM_HEADERS: -fmodules-validate-system-headers
 
-// RUN: %clang -fno-modules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID %s
+// RUN: %clang -fmodules -fno-modules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID %s
 // MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID-NOT: -fmodules-validate-system-headers
 
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT %s
 // MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT-NOT: -fmodules-disable-diagnostic-validation
 
-// RUN: %clang -fmodules-disable-diagnostic-validation -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION %s
+// RUN: %clang -fmodules -fmodules-disable-diagnostic-validation -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGN

[PATCH] D129030: [Driver] Ignore the clang modules validation-related flags if clang modules are not enabled

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG93d6fdfc232c: [Driver] Ignore the clang modules 
validation-related flags if clang modules are… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129030/new/

https://reviews.llvm.org/D129030

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/modules.m

Index: clang/test/Driver/modules.m
===
--- clang/test/Driver/modules.m
+++ clang/test/Driver/modules.m
@@ -4,27 +4,27 @@
 // RUN: %clang -fmodules -fno-modules -fmodules -### %s 2>&1 | FileCheck -check-prefix=CHECK-HAS-MODULES %s
 // CHECK-HAS-MODULES: -fmodules
 
-// RUN: %clang -fbuild-session-file=doesntexist -### %s 2>&1 | FileCheck -check-prefix=NOFILE %s
+// RUN: %clang -fmodules -fbuild-session-file=doesntexist -### %s 2>&1 | FileCheck -check-prefix=NOFILE %s
 // NOFILE: no such file or directory: 'doesntexist'
 
 // RUN: touch -m -a -t 201008011501 %t.build-session-file
-// RUN: %clang -fbuild-session-file=%t.build-session-file -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
 
-// RUN: %clang -fbuild-session-timestamp=1280703457 -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
+// RUN: %clang -fmodules -fbuild-session-timestamp=1280703457 -### %s 2>&1 | FileCheck -check-prefix=TIMESTAMP_ONLY %s
 // TIMESTAMP_ONLY: -fbuild-session-timestamp=128{{([[:digit:]]{7})[^[:digit:]]}}
 
-// RUN: %clang -fbuild-session-file=%t.build-session-file -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=CONFLICT %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -fbuild-session-timestamp=123 -### %s 2>&1 | FileCheck -check-prefix=CONFLICT %s
 // CONFLICT: error: invalid argument '-fbuild-session-file={{.*}}.build-session-file' not allowed with '-fbuild-session-timestamp'
 
-// RUN: %clang -fbuild-session-timestamp=123 -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE %s
+// RUN: %clang -fmodules -fbuild-session-timestamp=123 -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE %s
 // MODULES_VALIDATE_ONCE: -fbuild-session-timestamp=123
 // MODULES_VALIDATE_ONCE: -fmodules-validate-once-per-build-session
 
-// RUN: %clang -fbuild-session-file=%t.build-session-file -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_FILE %s
+// RUN: %clang -fmodules -fbuild-session-file=%t.build-session-file -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_FILE %s
 // MODULES_VALIDATE_ONCE_FILE: -fbuild-session-timestamp=128{{([[:digit:]]{7})[^[:digit:]]}}
 // MODULES_VALIDATE_ONCE_FILE: -fmodules-validate-once-per-build-session
 
-// RUN: %clang -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_ERR %s
+// RUN: %clang -fmodules -fmodules-validate-once-per-build-session -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_ONCE_ERR %s
 // MODULES_VALIDATE_ONCE_ERR: option '-fmodules-validate-once-per-build-session' requires '-fbuild-session-timestamp=' or '-fbuild-session-file='
 
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT %s
@@ -33,16 +33,16 @@
 // RUN: %clang -fmodules -fsyntax-only -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT_MOD %s
 // MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT_MOD: -fmodules-validate-system-headers
 
-// RUN: %clang -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS %s
+// RUN: %clang -fmodules -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS %s
 // MODULES_VALIDATE_SYSTEM_HEADERS: -fmodules-validate-system-headers
 
-// RUN: %clang -fno-modules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID %s
+// RUN: %clang -fmodules -fno-modules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID %s
 // MODULES_VALIDATE_SYSTEM_HEADERS_NOSYSVALID-NOT: -fmodules-validate-system-headers
 
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT %s
 // MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT-NOT: -fmodules-disable-diagnostic-validation
 
-// RUN: %clang -fmodules-disable-diagnostic-validation -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION %s
+// RUN: %clang -fmodules -fmodules-disable-diagnostic-validation -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALID

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added subscribers: ayermolo, sdasgup3, wenzhicui, wrengr, Chia-hungDuan, 
dcaballe, cota, teijeong, dexonsmith, rdzhabarov, tatianashp, msifontes, 
jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, 
antiagainst, shauheen, rriddle, mehdi_amini, hiraditya, arichardson, emaste.
Herald added a reviewer: jhenderson.
Herald added a reviewer: rriddle.
Herald added a reviewer: MaskRay.
Herald added a reviewer: rafauler.
Herald added a reviewer: Amir.
Herald added a reviewer: maksfb.
Herald added projects: lld-macho, All.
Herald added a reviewer: lld-macho.
akyrtzi requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, yota9, StephenFan, 
stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, MLIR, LLVM.

Returning `std::array` is better ergonomics for the hashing 
functions usage, instead of a `StringRef`:

- When returning `StringRef`, client code is "jumping through hoops" to do 
string manipulations instead of dealing with array of bytes directly, which is 
more natural
- Returning `std::array` avoids the need for the hasher classes to 
keep a field just for the purpose of wrapping it and returning it as a 
`StringRef`

As part of this patch also change the `BLAKE3` class API to match 
`HashBuilder`'s generic hash API.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/ELF/Writer.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

STAMPS
actor(@akyrtzi) application(Differential) author(@akyrtzi) herald(H105) 
herald(H123) herald(H208) herald(H224) herald(H225) herald(H311) herald(H423) 
herald(H452) herald(H530) herald(H532) herald(H533) herald(H538) herald(H540) 
herald(H541) herald(H544) herald(H546) herald(H547) herald(H550) herald(H551) 
herald(H554) herald(H557) herald(H560) herald(H561) herald(H565) herald(H576) 
herald(H592) herald(H597) herald(H598) herald(H607) herald(H610) herald(H615) 
herald(H625) herald(H629) herald(H645) herald(H672) herald(H685) herald(H688) 
herald(H698) herald(H723) herald(H727) herald(H729) herald(H739) herald(H802) 
herald(H805) herald(H809) herald(H827) herald(H842) herald(H844) herald(H845) 
herald(H847) herald(H854) herald(H855) herald(H861) herald(H864) 
monogram(D123100) object-type(DREV) phid(PHID-DREV-ngvkfimyomxukhcmhald) 
reviewer(#lld-macho) reviewer(@Amir) reviewer(@jhenderson) reviewer(@maksfb) 
reviewer(@MaskRay) reviewer(@rafauler) reviewer(@rriddle) 
revision-repository(rG) revision-status(needs-review) subscriber(@aartbik) 
subscriber(@antiagainst) subscriber(@arichardson) subscriber(@arpith-jacob) 
subscriber(@ayermolo) subscriber(@cfe-commits) subscriber(@Chia-hungDuan) 
subscriber(@cota) subscriber(@dcaballe) subscriber(@dexonsmith) 
subscriber(@emaste) subscriber(@grosul1) subscriber(@hiraditya) 
subscriber(@Joonsoo) subscriber(@jurahul) subscriber(@Kayjukh) 
subscriber(@liufengdb) subscriber(@llvm-commits) subscriber(@mehdi_amini) 
subscriber(@mgester) subscriber(@msifontes) subscriber(@nicolasvasilache) 
subscriber(@rdzhabarov) subscriber(@rriddle) subscriber(@sdasgup3) 
subscriber(@shauheen) subscriber(@StephenFan) subscriber(@stephenneuendorffer) 
subscriber(@tatianashp) subscriber(@teijeong) subscriber(@wenzhicui) 
subscriber(@wrengr) subscriber(@yota9) tag(#all) tag(#clang) tag(#lld-macho) 
tag(#llvm) tag(#mlir) via(conduit)

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Inpu

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 420359.
akyrtzi added a comment.

Adjust `ASTFileSignature` to accept only the array hash bytes and directly from 
the `final()` call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/ELF/Writer.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

STAMPS
actor(@akyrtzi) application(Differential) author(@akyrtzi) herald(H105) 
herald(H123) herald(H208) herald(H224) herald(H225) herald(H311) herald(H423) 
herald(H452) herald(H530) herald(H532) herald(H533) herald(H538) herald(H540) 
herald(H541) herald(H544) herald(H546) herald(H547) herald(H550) herald(H551) 
herald(H554) herald(H557) herald(H560) herald(H561) herald(H565) herald(H576) 
herald(H592) herald(H597) herald(H598) herald(H607) herald(H610) herald(H615) 
herald(H625) herald(H629) herald(H645) herald(H672) herald(H685) herald(H688) 
herald(H698) herald(H723) herald(H727) herald(H729) herald(H739) herald(H802) 
herald(H805) herald(H809) herald(H827) herald(H842) herald(H844) herald(H845) 
herald(H847) herald(H854) herald(H855) herald(H861) herald(H864) 
monogram(D123100) object-type(DREV) phid(PHID-DREV-ngvkfimyomxukhcmhald) 
reviewer(#lld-macho) reviewer(@Amir) reviewer(@dexonsmith) 
reviewer(@jhenderson) reviewer(@maksfb) reviewer(@MaskRay) reviewer(@rafauler) 
reviewer(@rriddle) revision-repository(rG) revision-status(accepted) 
subscriber(@aartbik) subscriber(@antiagainst) subscriber(@arichardson) 
subscriber(@arpith-jacob) subscriber(@ayermolo) subscriber(@cfe-commits) 
subscriber(@Chia-hungDuan) subscriber(@cota) subscriber(@dcaballe) 
subscriber(@dexonsmith) subscriber(@emaste) subscriber(@grosul1) 
subscriber(@hiraditya) subscriber(@Joonsoo) subscriber(@jurahul) 
subscriber(@Kayjukh) subscriber(@liufengdb) subscriber(@llvm-commits) 
subscriber(@mehdi_amini) subscriber(@mgester) subscriber(@msifontes) 
subscriber(@nicolasvasilache) subscriber(@rdzhabarov) subscriber(@rriddle) 
subscriber(@sdasgup3) subscriber(@shauheen) subscriber(@StephenFan) 
subscriber(@stephenneuendorffer) subscriber(@tatianashp) subscriber(@teijeong) 
subscriber(@wenzhicui) subscriber(@wrengr) subscriber(@yota9) tag(#all) 
tag(#clang) tag(#lld-macho) tag(#llvm) tag(#mlir) via(conduit)

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789ABCDEF";
   size_t Length = Input.size();
 
@@ -39,7 +39,7 @@
 TEST(sha1_hash_test, Basic) {
   ArrayRef Input((const uint8_t *)"Hello World!", 12);
   std::array Vec = SHA1::hash(Input);
-  std::string Hash = toHex({(const char *)Vec.data(), 20});
+  std::string Hash = toHex(Vec);
   ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
 }
 
Index: llvm/unittests/Support/SHA256.cpp
===
--- llvm/unittests/Support/SHA256.cpp
+++ llvm/unittests/Support/SHA256.cpp
@@ -20,7 +20,7 @@
 
 namespace {
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789abcdef";
   size_t Length = Input.size();
 
Index: llvm/unittests/Support/MD5Test.cpp
===
--- llvm/unittests/Support/MD5Test.cpp
+++ llvm/unittests/Support/MD5Test.cpp
@@ -79,10 +79,7 @@
 ReferenceHash.update("abcd");
 MD5::MD5Result ReferenceResult;
 Refere

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 420377.
akyrtzi added a comment.

- Move `BLAKE3` back to templated sizes for `final()` and `result()` and add 
`TruncatedBLAKE3` that has the size parameter on the class.
- Make `MD5Result` inherit from `std::array` which both simplifies 
its API and makes it a better fit for replacing `StringRef`.
- Use `HASH_LENGTH` in implementations of `SHA1::final()` and `SHA256::final()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/lib/MC/MCDwarf.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789ABCDEF";
   size_t Length = Input.size();
 
@@ -39,7 +39,7 @@
 TEST(sha1_hash_test, Basic) {
   ArrayRef Input((const uint8_t *)"Hello World!", 12);
   std::array Vec = SHA1::hash(Input);
-  std::string Hash = toHex({(const char *)Vec.data(), 20});
+  std::string Hash = toHex(Vec);
   ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
 }
 
Index: llvm/unittests/Support/SHA256.cpp
===
--- llvm/unittests/Support/SHA256.cpp
+++ llvm/unittests/Support/SHA256.cpp
@@ -20,7 +20,7 @@
 
 namespace {
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789abcdef";
   size_t Length = Input.size();
 
Index: llvm/unittests/Support/MD5Test.cpp
===
--- llvm/unittests/Support/MD5Test.cpp
+++ llvm/unittests/Support/MD5Test.cpp
@@ -62,7 +62,7 @@
   std::array Vec = MD5::hash(Input);
   MD5::MD5Result MD5Res;
   SmallString<32> Res;
-  memcpy(MD5Res.Bytes.data(), Vec.data(), Vec.size());
+  memcpy(MD5Res.data(), Vec.data(), Vec.size());
   MD5::stringifyResult(MD5Res, Res);
   EXPECT_EQ(Res, "c3fcd3d76192e4007dfb496cca67e13b");
   EXPECT_EQ(0x3be167ca6c49fb7dULL, MD5Res.high());
@@ -79,10 +79,7 @@
 ReferenceHash.update("abcd");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.result(), ExpectedResult);
+EXPECT_EQ(Hash.result(), ReferenceResult);
   }
 
   Hash.update("xyz");
@@ -93,10 +90,7 @@
 ReferenceHash.update("xyz");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.final(), ExpectedResult);
+EXPECT_EQ(Hash.final(), ReferenceResult);
   }
 }
 } // namespace
Index: llvm/unittests/Support/HashBuilderTest.cpp
===
--- llvm/unittests/Support/HashBuilderTest.cpp
+++ llvm/unittests/Support/HashBuilderTest.cpp
@@ -44,13 +44,15 @@
   HasherTAndEndianness::Endianness>;
 
 template 
-static std::string hashWithBuilder(const Ts &...Args) {
-  return HashBuilder().add(Args...).final().str();
+sta

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: llvm/include/llvm/Support/BLAKE3.h:38
+/// A class that wraps the BLAKE3 algorithm.
+template  class BLAKE3 {
 public:

dexonsmith wrote:
> The visual noise of `BLAKE3<>` everywhere is a bit unfortunate.
> 
> Another option:
> ```
> lang=c++
> // Hardcoded to 32B. OR same implementation as before, with optional template
> // parameters to choose a size when accessing the hash.
> class BLAKE3 { /* ... */ };
> 
> // Wrap final(), result(), and hash() to truncate the hash.
> template  class TruncatedBLAKE3 : public BLAKE3 { 
> ... };
> ```
> 
> WDYT?
Good idea! I moved `BLAKE3` back to templated sizes for `final()` and 
`result()` and added `TruncatedBLAKE3` that has the size parameter on the class.
`TruncatedBLAKE3` is useful for using BLAKE3 as the hasher type for 
`HashBuilder` with non-default hash sizes, otherwise one can use `BLAKE3` for 
all other uses.



Comment at: llvm/include/llvm/Support/MD5.h:82
+  /// Finishes off the hash, and returns the 16-byte hash data.
+  std::array final();
 

dexonsmith wrote:
> Should this (and `result()` below) be `MD5Result`? It has an implicit cast to 
> `std::array`. Maybe it's better as you have it... not sure...
I avoided `MD5Result` due to not being as good as `std::array` for a "drop-in 
replacement" for `StringRef` due to lack of `data()` and `size()`.
But it occurred to me that `MD5Result` could just inherit from 
`std::array` which would make it better fit to replace `StringRef` 
//and// improves & simplifies its API, see updated patch.



Comment at: llvm/lib/Support/SHA1.cpp:289
+std::array HashResult;
+std::array ReturnResult;
+  };

dexonsmith wrote:
> Should this be `HASH_LENGTH` instead of 20?
Updated 👍


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 420378.
akyrtzi added a comment.

Also revert the `README` changes to the previous version of `BLAKE3` class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/lib/MC/MCDwarf.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789ABCDEF";
   size_t Length = Input.size();
 
@@ -39,7 +39,7 @@
 TEST(sha1_hash_test, Basic) {
   ArrayRef Input((const uint8_t *)"Hello World!", 12);
   std::array Vec = SHA1::hash(Input);
-  std::string Hash = toHex({(const char *)Vec.data(), 20});
+  std::string Hash = toHex(Vec);
   ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
 }
 
Index: llvm/unittests/Support/SHA256.cpp
===
--- llvm/unittests/Support/SHA256.cpp
+++ llvm/unittests/Support/SHA256.cpp
@@ -20,7 +20,7 @@
 
 namespace {
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789abcdef";
   size_t Length = Input.size();
 
Index: llvm/unittests/Support/MD5Test.cpp
===
--- llvm/unittests/Support/MD5Test.cpp
+++ llvm/unittests/Support/MD5Test.cpp
@@ -62,7 +62,7 @@
   std::array Vec = MD5::hash(Input);
   MD5::MD5Result MD5Res;
   SmallString<32> Res;
-  memcpy(MD5Res.Bytes.data(), Vec.data(), Vec.size());
+  memcpy(MD5Res.data(), Vec.data(), Vec.size());
   MD5::stringifyResult(MD5Res, Res);
   EXPECT_EQ(Res, "c3fcd3d76192e4007dfb496cca67e13b");
   EXPECT_EQ(0x3be167ca6c49fb7dULL, MD5Res.high());
@@ -79,10 +79,7 @@
 ReferenceHash.update("abcd");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.result(), ExpectedResult);
+EXPECT_EQ(Hash.result(), ReferenceResult);
   }
 
   Hash.update("xyz");
@@ -93,10 +90,7 @@
 ReferenceHash.update("xyz");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.final(), ExpectedResult);
+EXPECT_EQ(Hash.final(), ReferenceResult);
   }
 }
 } // namespace
Index: llvm/unittests/Support/HashBuilderTest.cpp
===
--- llvm/unittests/Support/HashBuilderTest.cpp
+++ llvm/unittests/Support/HashBuilderTest.cpp
@@ -44,13 +44,15 @@
   HasherTAndEndianness::Endianness>;
 
 template 
-static std::string hashWithBuilder(const Ts &...Args) {
-  return HashBuilder().add(Args...).final().str();
+static typename HashBuilder::template HashResultTy<>
+hashWithBuilder(const Ts &...Args) {
+  return HashBuilder().add(Args...).final();
 }
 
 template 
-static std::string hashRangeWithBuilder(const Ts &...Args) {
-  return HashBuilder().addRange(Args...).final().str();
+s

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 420545.
akyrtzi added a comment.

Expand type instead of using `auto`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/lib/MC/MCDwarf.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789ABCDEF";
   size_t Length = Input.size();
 
@@ -39,7 +39,7 @@
 TEST(sha1_hash_test, Basic) {
   ArrayRef Input((const uint8_t *)"Hello World!", 12);
   std::array Vec = SHA1::hash(Input);
-  std::string Hash = toHex({(const char *)Vec.data(), 20});
+  std::string Hash = toHex(Vec);
   ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
 }
 
Index: llvm/unittests/Support/SHA256.cpp
===
--- llvm/unittests/Support/SHA256.cpp
+++ llvm/unittests/Support/SHA256.cpp
@@ -20,7 +20,7 @@
 
 namespace {
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789abcdef";
   size_t Length = Input.size();
 
Index: llvm/unittests/Support/MD5Test.cpp
===
--- llvm/unittests/Support/MD5Test.cpp
+++ llvm/unittests/Support/MD5Test.cpp
@@ -62,7 +62,7 @@
   std::array Vec = MD5::hash(Input);
   MD5::MD5Result MD5Res;
   SmallString<32> Res;
-  memcpy(MD5Res.Bytes.data(), Vec.data(), Vec.size());
+  memcpy(MD5Res.data(), Vec.data(), Vec.size());
   MD5::stringifyResult(MD5Res, Res);
   EXPECT_EQ(Res, "c3fcd3d76192e4007dfb496cca67e13b");
   EXPECT_EQ(0x3be167ca6c49fb7dULL, MD5Res.high());
@@ -79,10 +79,7 @@
 ReferenceHash.update("abcd");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.result(), ExpectedResult);
+EXPECT_EQ(Hash.result(), ReferenceResult);
   }
 
   Hash.update("xyz");
@@ -93,10 +90,7 @@
 ReferenceHash.update("xyz");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.final(), ExpectedResult);
+EXPECT_EQ(Hash.final(), ReferenceResult);
   }
 }
 } // namespace
Index: llvm/unittests/Support/HashBuilderTest.cpp
===
--- llvm/unittests/Support/HashBuilderTest.cpp
+++ llvm/unittests/Support/HashBuilderTest.cpp
@@ -44,13 +44,15 @@
   HasherTAndEndianness::Endianness>;
 
 template 
-static std::string hashWithBuilder(const Ts &...Args) {
-  return HashBuilder().add(Args...).final().str();
+static typename HashBuilder::template HashResultTy<>
+hashWithBuilder(const Ts &...Args) {
+  return HashBuilder().add(Args...).final();
 }
 
 template 
-static std::string hashRangeWithBuilder(const Ts &...Args) {
-  return HashBuilder().addRange(Args...).final().str();
+static typename HashBuilder::template Has

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: bolt/lib/Core/DebugData.cpp:823
 Hasher.update(AbbrevData);
-StringRef Key = Hasher.final();
+auto Hash = Hasher.final();
+StringRef Key((const char *)Hash.data(), Hash.size());

jhenderson wrote:
> Amir wrote:
> > I think it would be more in line with LLVM coding standards to expand 
> > `auto` in this case (and others) – see 
> > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable.
> >  
> > My understanding is that `auto` is fine where the type is obvious from the 
> > context (is explicitly available on the same line e.g. with casts), is 
> > abstract (T::iterator types), or doesn't matter (e.g. lambdas).
> +1 (also relevant in other places in this patch)
Updated 👍


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG330268ba346b: [Support/Hash functions] Change the `final()` 
and `result()` of the hashing… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123100/new/

https://reviews.llvm.org/D123100

Files:
  bolt/lib/Core/DebugData.cpp
  clang/include/clang/Basic/Module.h
  clang/lib/Serialization/ASTWriter.cpp
  lld/MachO/SyntheticSections.cpp
  llvm/include/llvm/Support/BLAKE3.h
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/MD5.h
  llvm/include/llvm/Support/SHA1.h
  llvm/include/llvm/Support/SHA256.h
  llvm/include/llvm/Support/raw_sha1_ostream.h
  llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/DebugInfo/CodeView/TypeHashing.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/lib/MC/MCDwarf.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/ObjCopy/MachO/MachOWriter.cpp
  llvm/lib/Support/BLAKE3/README.md
  llvm/lib/Support/MD5.cpp
  llvm/lib/Support/SHA1.cpp
  llvm/lib/Support/SHA256.cpp
  llvm/unittests/Support/BLAKE3Test.cpp
  llvm/unittests/Support/HashBuilderTest.cpp
  llvm/unittests/Support/MD5Test.cpp
  llvm/unittests/Support/SHA256.cpp
  llvm/unittests/Support/raw_sha1_ostream_test.cpp
  mlir/lib/Pass/IRPrinting.cpp

Index: mlir/lib/Pass/IRPrinting.cpp
===
--- mlir/lib/Pass/IRPrinting.cpp
+++ mlir/lib/Pass/IRPrinting.cpp
@@ -66,7 +66,7 @@
 ArrayRef(reinterpret_cast(&data), sizeof(T)));
   }
 
-  SmallString<20> hash;
+  std::array hash;
 };
 
 //===--===//
Index: llvm/unittests/Support/raw_sha1_ostream_test.cpp
===
--- llvm/unittests/Support/raw_sha1_ostream_test.cpp
+++ llvm/unittests/Support/raw_sha1_ostream_test.cpp
@@ -14,7 +14,7 @@
 
 using namespace llvm;
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789ABCDEF";
   size_t Length = Input.size();
 
@@ -39,7 +39,7 @@
 TEST(sha1_hash_test, Basic) {
   ArrayRef Input((const uint8_t *)"Hello World!", 12);
   std::array Vec = SHA1::hash(Input);
-  std::string Hash = toHex({(const char *)Vec.data(), 20});
+  std::string Hash = toHex(Vec);
   ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash);
 }
 
Index: llvm/unittests/Support/SHA256.cpp
===
--- llvm/unittests/Support/SHA256.cpp
+++ llvm/unittests/Support/SHA256.cpp
@@ -20,7 +20,7 @@
 
 namespace {
 
-static std::string toHex(StringRef Input) {
+static std::string toHex(ArrayRef Input) {
   static const char *const LUT = "0123456789abcdef";
   size_t Length = Input.size();
 
Index: llvm/unittests/Support/MD5Test.cpp
===
--- llvm/unittests/Support/MD5Test.cpp
+++ llvm/unittests/Support/MD5Test.cpp
@@ -62,7 +62,7 @@
   std::array Vec = MD5::hash(Input);
   MD5::MD5Result MD5Res;
   SmallString<32> Res;
-  memcpy(MD5Res.Bytes.data(), Vec.data(), Vec.size());
+  memcpy(MD5Res.data(), Vec.data(), Vec.size());
   MD5::stringifyResult(MD5Res, Res);
   EXPECT_EQ(Res, "c3fcd3d76192e4007dfb496cca67e13b");
   EXPECT_EQ(0x3be167ca6c49fb7dULL, MD5Res.high());
@@ -79,10 +79,7 @@
 ReferenceHash.update("abcd");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.result(), ExpectedResult);
+EXPECT_EQ(Hash.result(), ReferenceResult);
   }
 
   Hash.update("xyz");
@@ -93,10 +90,7 @@
 ReferenceHash.update("xyz");
 MD5::MD5Result ReferenceResult;
 ReferenceHash.final(ReferenceResult);
-StringRef ExpectedResult =
-StringRef(reinterpret_cast(ReferenceResult.Bytes.data()),
-  ReferenceResult.Bytes.size());
-EXPECT_EQ(Hash.final(), ExpectedResult);
+EXPECT_EQ(Hash.final(), ReferenceResult);
   }
 }
 } // namespace
Index: llvm/unittests/Support/HashBuilderTest.cpp
===
--- llvm/unittests/Support/HashBuilderTest.cpp
+++ llvm/unittests/Support/HashBuilderTest.cpp
@@ -44,13 +44,15 @@
   HasherTAndEndianness::Endianness>;
 
 template 
-static std::string hashWithBuilder(const Ts &...Args) {
-  return HashBuilder().add(Args...).final().str();
+static typename HashBuilder::template HashResultTy<>
+hashWithBuilder(const Ts &...Args) {
+  return HashBuilder().add(Args...).final();
 }
 
 template 
-stati

[PATCH] D130443: [CGDebugInfo] Access the current working directory from the `VFS`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8dfaecc4c244: [CGDebugInfo] Access the current working 
directory from the `VFS` (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130443/new/

https://reviews.llvm.org/D130443

Files:
  clang/include/clang/CodeGen/ModuleBuilder.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/unittests/CodeGen/TestCompiler.h
  clang/unittests/Frontend/CodeGenActionTest.cpp

Index: clang/unittests/Frontend/CodeGenActionTest.cpp
===
--- clang/unittests/Frontend/CodeGenActionTest.cpp
+++ clang/unittests/Frontend/CodeGenActionTest.cpp
@@ -15,6 +15,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -76,4 +77,40 @@
   bool Success = Compiler.ExecuteAction(Action);
   EXPECT_TRUE(Success);
 }
+
+TEST(CodeGenTest, DebugInfoCWDCodeGen) {
+  // Check that debug info is accessing the current working directory from the
+  // VFS instead of calling \p llvm::sys::fs::current_path() directly.
+
+  auto VFS = std::make_unique();
+  VFS->setCurrentWorkingDirectory("/in-memory-fs-cwd");
+  auto Sept = llvm::sys::path::get_separator();
+  std::string TestPath =
+  std::string(llvm::formatv("{0}in-memory-fs-cwd{0}test.cpp", Sept));
+  VFS->addFile(TestPath, 0, llvm::MemoryBuffer::getMemBuffer("int x;\n"));
+
+  auto Invocation = std::make_shared();
+  Invocation->getFrontendOpts().Inputs.push_back(
+  FrontendInputFile("test.cpp", Language::CXX));
+  Invocation->getFrontendOpts().ProgramAction = EmitLLVM;
+  Invocation->getTargetOpts().Triple = "x86_64-unknown-linux-gnu";
+  Invocation->getCodeGenOpts().setDebugInfo(codegenoptions::FullDebugInfo);
+  CompilerInstance Compiler;
+
+  SmallString<256> IRBuffer;
+  Compiler.setOutputStream(std::make_unique(IRBuffer));
+  Compiler.setInvocation(std::move(Invocation));
+  Compiler.createDiagnostics();
+  Compiler.createFileManager(std::move(VFS));
+
+  EmitLLVMAction Action;
+  bool Success = Compiler.ExecuteAction(Action);
+  EXPECT_TRUE(Success);
+
+  SmallString<128> RealCWD;
+  llvm::sys::fs::current_path(RealCWD);
+  EXPECT_TRUE(!RealCWD.empty());
+  EXPECT_FALSE(IRBuffer.str().contains(RealCWD));
+  EXPECT_TRUE(IRBuffer.str().contains("in-memory-fs-cwd"));
+}
 }
Index: clang/unittests/CodeGen/TestCompiler.h
===
--- clang/unittests/CodeGen/TestCompiler.h
+++ clang/unittests/CodeGen/TestCompiler.h
@@ -56,12 +56,10 @@
 
 compiler.createASTContext();
 
-CG.reset(CreateLLVMCodeGen(compiler.getDiagnostics(),
-   "main-module",
-   compiler.getHeaderSearchOpts(),
-   compiler.getPreprocessorOpts(),
-   compiler.getCodeGenOpts(),
-   Context));
+CG.reset(CreateLLVMCodeGen(
+compiler.getDiagnostics(), "main-module",
+&compiler.getVirtualFileSystem(), compiler.getHeaderSearchOpts(),
+compiler.getPreprocessorOpts(), compiler.getCodeGenOpts(), Context));
   }
 
   void init(const char *TestProgram,
Index: clang/tools/clang-import-test/clang-import-test.cpp
===
--- clang/tools/clang-import-test/clang-import-test.cpp
+++ clang/tools/clang-import-test/clang-import-test.cpp
@@ -230,8 +230,9 @@
 llvm::LLVMContext &LLVMCtx) {
   StringRef ModuleName("$__module");
   return std::unique_ptr(CreateLLVMCodeGen(
-  CI.getDiagnostics(), ModuleName, CI.getHeaderSearchOpts(),
-  CI.getPreprocessorOpts(), CI.getCodeGenOpts(), LLVMCtx));
+  CI.getDiagnostics(), ModuleName, &CI.getVirtualFileSystem(),
+  CI.getHeaderSearchOpts(), CI.getPreprocessorOpts(), CI.getCodeGenOpts(),
+  LLVMCtx));
 }
 } // namespace init_convenience
 
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -45,6 +45,7 @@
   const std::string OutputFileName;
   ASTContext *Ctx;
   ModuleMap &MMap;
+  IntrusiveRefCntPtr FS;
   const HeaderSearchOptions &HeaderSearchOpts;
   const PreprocessorOptions &PreprocessorOpts;
   CodeGenOptions CodeGenOpts;
@@ -144,6 +145,7 @@
   : Diags

[PATCH] D130443: [CGDebugInfo] Access the current working directory from the `VFS`

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D130443#3680753 , @thakis wrote:

> Looks like this doesn't build: http://45.33.8.238/linux/82380/step_4.txt

Sorry about that, fixed here: 
https://github.com/llvm/llvm-project/commit/c5ddacb3b6afe2fd507b5f4a10c32ec00ffb245e


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130443/new/

https://reviews.llvm.org/D130443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is useful to enable sharing of the same PCH file even when it's intended 
for a different output path.

The only information this option disables writing is for `ORIGINAL_PCH_DIR` 
record which is treated as optional and (when present) used as fallback for 
resolving input file paths relative to it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130710

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/test/PCH/pch-output-path-independent.c

Index: clang/test/PCH/pch-output-path-independent.c
===
--- /dev/null
+++ clang/test/PCH/pch-output-path-independent.c
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t && mkdir -p %t/a %t/b
+
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch -fpcm-output-path-independent
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch -fpcm-output-path-independent
+
+// RUN: diff %t/a/t1.pch %t/b/t2.pch
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -25,13 +25,14 @@
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
 bool AllowASTWithErrors, bool IncludeTimestamps,
-bool ShouldCacheASTInMemory)
+bool ShouldCacheASTInMemory, bool OutputPathIndependent)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
   AllowASTWithErrors(AllowASTWithErrors),
-  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory),
+  OutputPathIndependent(OutputPathIndependent) {
   this->Buffer->IsComplete = false;
 }
 
@@ -70,7 +71,7 @@
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
   PP.getDiagnostics().hasUncompilableErrorOccurred(),
-  ShouldCacheASTInMemory);
+  ShouldCacheASTInMemory, OutputPathIndependent);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4483,7 +4483,8 @@
 ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef, StringRef OutputFile,
  Module *WritingModule, StringRef isysroot,
  bool hasErrors,
- bool ShouldCacheASTInMemory) {
+ bool ShouldCacheASTInMemory,
+ bool OutputPathIndependent) {
   WritingAST = true;
 
   ASTHasCompilerErrors = hasErrors;
@@ -4499,8 +4500,9 @@
   Context = &SemaRef.Context;
   PP = &SemaRef.PP;
   this->WritingModule = WritingModule;
-  ASTFileSignature Signature =
-  WriteASTCore(SemaRef, isysroot, OutputFile, WritingModule);
+  ASTFileSignature Signature = WriteASTCore(
+  SemaRef, isysroot, OutputPathIndependent ? StringRef() : OutputFile,
+  WritingModule);
   Context = nullptr;
   PP = nullptr;
   this->WritingModule = nullptr;
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,8 @@
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   FrontendOpts.ModuleFileExtensions,
   CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
+  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH,
+  FrontendOpts.OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
 
@@ -203,7 +204,9 @@
   /*IncludeTimestamps=*/
   +CI.getFrontendOpts().BuildingImplicitModule,
   /*ShouldCacheASTInMemory=*/
-  +CI.getFrontendOpts().BuildingImplicitModule));
+  +CI.getFrontendOpts().BuildingImplicitModule,
+  /*OutputPathIndependent=*/
+  +CI.getFrontendOpts().OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGe

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a subscriber: v.g.vassilev.
akyrtzi added a comment.

@v.g.vassilev is the functionality of "write `ORIGINAL_PCH_DIR` and resolve 
headers relative to it if PCH file and headers moved together" used by `Cling`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a subscriber: rmaz.
akyrtzi added a comment.

Also pinging @rmaz who made a related change 
, is this used by Facebook?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D130710#3685509 , @v.g.vassilev 
wrote:

> In D130710#3685470 , @akyrtzi wrote:
>
>> @v.g.vassilev is the functionality of "write `ORIGINAL_PCH_DIR` and resolve 
>> headers relative to it if PCH file and headers moved together" used by 
>> `Cling`?
>
> We currently use `-fmodules-embed-all-files` which zips all header files in a 
> blob in the PCH/PCM that gives us flexibility to not require the headers to 
> be present at a specific location. We have mostly moved to PCMs and the PCH 
> is of less interest to us. Does that patch help making the implicitly built 
> module files easier to relocate?

Not sure whether `ORIGINAL_PCH_DIR` relates to implicitly built module files or 
not, I'm wondering whether `ORIGINAL_PCH_DIR` is still useful and, if I 
understand correctly, it's not useful for `Cling` since it embeds the header 
file contents, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 448715.
akyrtzi added a comment.

Add `FIXME` comment to consider either removing `ORIGINAL_PCH_DIR` or changing 
the default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/test/PCH/pch-output-path-independent.c

Index: clang/test/PCH/pch-output-path-independent.c
===
--- /dev/null
+++ clang/test/PCH/pch-output-path-independent.c
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t && mkdir -p %t/a %t/b
+
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch -fpcm-output-path-independent
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch -fpcm-output-path-independent
+
+// RUN: diff %t/a/t1.pch %t/b/t2.pch
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -25,13 +25,14 @@
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
 bool AllowASTWithErrors, bool IncludeTimestamps,
-bool ShouldCacheASTInMemory)
+bool ShouldCacheASTInMemory, bool OutputPathIndependent)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
   AllowASTWithErrors(AllowASTWithErrors),
-  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory),
+  OutputPathIndependent(OutputPathIndependent) {
   this->Buffer->IsComplete = false;
 }
 
@@ -70,7 +71,7 @@
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
   PP.getDiagnostics().hasUncompilableErrorOccurred(),
-  ShouldCacheASTInMemory);
+  ShouldCacheASTInMemory, OutputPathIndependent);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4483,7 +4483,8 @@
 ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef, StringRef OutputFile,
  Module *WritingModule, StringRef isysroot,
  bool hasErrors,
- bool ShouldCacheASTInMemory) {
+ bool ShouldCacheASTInMemory,
+ bool OutputPathIndependent) {
   WritingAST = true;
 
   ASTHasCompilerErrors = hasErrors;
@@ -4499,8 +4500,9 @@
   Context = &SemaRef.Context;
   PP = &SemaRef.PP;
   this->WritingModule = WritingModule;
-  ASTFileSignature Signature =
-  WriteASTCore(SemaRef, isysroot, OutputFile, WritingModule);
+  ASTFileSignature Signature = WriteASTCore(
+  SemaRef, isysroot, OutputPathIndependent ? StringRef() : OutputFile,
+  WritingModule);
   Context = nullptr;
   PP = nullptr;
   this->WritingModule = nullptr;
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,8 @@
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   FrontendOpts.ModuleFileExtensions,
   CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
+  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH,
+  FrontendOpts.OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
 
@@ -203,7 +204,9 @@
   /*IncludeTimestamps=*/
   +CI.getFrontendOpts().BuildingImplicitModule,
   /*ShouldCacheASTInMemory=*/
-  +CI.getFrontendOpts().BuildingImplicitModule));
+  +CI.getFrontendOpts().BuildingImplicitModule,
+  /*OutputPathIndependent=*/
+  +CI.getFrontendOpts().OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
   return std::make_unique(std::move(Consumers));
Index: clang/include/clang/Serialization/ASTWriter.h
===

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D130710#3685436 , @benlangmuir 
wrote:

> Is the functionality provided by `ORIGINAL_PCH_DIR` still useful for making 
> it possible to move a PCH and its referenced headers?  It's not completely 
> clear to me when this feature is used in practice.  It would be nice to 
> remove it or change the default behaviour if possible, rather than require a 
> new option, but I'm open to this approach if we think we can't change the 
> default.

Given that there was a recent change  related 
to `ORIGINAL_PCH_DIR`, I'm reluctant to change the default at this point, I 
added a `FIXME` for following up to see if we can remove it or change the 
default later on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG944a86de7c50: [ASTWriter] Provide capability to output a 
PCM/PCH file that does not write out… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/test/PCH/pch-output-path-independent.c

Index: clang/test/PCH/pch-output-path-independent.c
===
--- /dev/null
+++ clang/test/PCH/pch-output-path-independent.c
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t && mkdir -p %t/a %t/b
+
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch -fpcm-output-path-independent
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch -fpcm-output-path-independent
+
+// RUN: diff %t/a/t1.pch %t/b/t2.pch
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -25,13 +25,14 @@
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
 bool AllowASTWithErrors, bool IncludeTimestamps,
-bool ShouldCacheASTInMemory)
+bool ShouldCacheASTInMemory, bool OutputPathIndependent)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
   AllowASTWithErrors(AllowASTWithErrors),
-  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory),
+  OutputPathIndependent(OutputPathIndependent) {
   this->Buffer->IsComplete = false;
 }
 
@@ -70,7 +71,7 @@
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
   PP.getDiagnostics().hasUncompilableErrorOccurred(),
-  ShouldCacheASTInMemory);
+  ShouldCacheASTInMemory, OutputPathIndependent);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4483,7 +4483,8 @@
 ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef, StringRef OutputFile,
  Module *WritingModule, StringRef isysroot,
  bool hasErrors,
- bool ShouldCacheASTInMemory) {
+ bool ShouldCacheASTInMemory,
+ bool OutputPathIndependent) {
   WritingAST = true;
 
   ASTHasCompilerErrors = hasErrors;
@@ -4499,8 +4500,9 @@
   Context = &SemaRef.Context;
   PP = &SemaRef.PP;
   this->WritingModule = WritingModule;
-  ASTFileSignature Signature =
-  WriteASTCore(SemaRef, isysroot, OutputFile, WritingModule);
+  ASTFileSignature Signature = WriteASTCore(
+  SemaRef, isysroot, OutputPathIndependent ? StringRef() : OutputFile,
+  WritingModule);
   Context = nullptr;
   PP = nullptr;
   this->WritingModule = nullptr;
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -140,7 +140,8 @@
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   FrontendOpts.ModuleFileExtensions,
   CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
+  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH,
+  FrontendOpts.OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
 
@@ -203,7 +204,9 @@
   /*IncludeTimestamps=*/
   +CI.getFrontendOpts().BuildingImplicitModule,
   /*ShouldCacheASTInMemory=*/
-  +CI.getFrontendOpts().BuildingImplicitModule));
+  +CI.getFrontendOpts().BuildingImplicitModule,
+  /*OutputPathIndependent=*/
+  +CI.getFrontendOpts().OutputPathIndependentPCM));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
   CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
   return std::make_unique(std::move(Consumers));
Index: clang/include/clang/Serialization/ASTWriter.h
==

[PATCH] D131124: [Serialization] Remove `ORIGINAL_PCH_DIR` record

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Use of `ORIGINAL_PCH_DIR` record has been superseeded by making PCH/PCM files 
with relocatable paths at write time.
Removing this record is useful for producing an output-path-independent PCH 
file and enable sharing of the same PCH file even
when it was intended for a different output path.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131124

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/test/Modules/relative-original-dir.m
  clang/test/PCH/pch-output-path-independent.c

Index: clang/test/PCH/pch-output-path-independent.c
===
--- clang/test/PCH/pch-output-path-independent.c
+++ clang/test/PCH/pch-output-path-independent.c
@@ -1,6 +1,6 @@
 // RUN: rm -rf %t && mkdir -p %t/a %t/b
 
-// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch -fpcm-output-path-independent
-// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch -fpcm-output-path-independent
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch
 
 // RUN: diff %t/a/t1.pch %t/b/t2.pch
Index: clang/test/Modules/relative-original-dir.m
===
--- clang/test/Modules/relative-original-dir.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t/normal-module-map
-// RUN: mkdir -p %t
-// RUN: cp -r %S/Inputs/normal-module-map %t
-// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA -emit-module %t/normal-module-map/module.map -o %t/normal-module-map/outdir/mod.pcm
-// RUN: llvm-bcanalyzer --dump --disable-histogram %t/normal-module-map/outdir/mod.pcm | FileCheck %s
-
-// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -25,14 +25,13 @@
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
 bool AllowASTWithErrors, bool IncludeTimestamps,
-bool ShouldCacheASTInMemory, bool OutputPathIndependent)
+bool ShouldCacheASTInMemory)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
   AllowASTWithErrors(AllowASTWithErrors),
-  ShouldCacheASTInMemory(ShouldCacheASTInMemory),
-  OutputPathIndependent(OutputPathIndependent) {
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
   this->Buffer->IsComplete = false;
 }
 
@@ -71,7 +70,7 @@
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
   PP.getDiagnostics().hasUncompilableErrorOccurred(),
-  ShouldCacheASTInMemory, OutputPathIndependent);
+  ShouldCacheASTInMemory);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -786,7 +786,6 @@
   RECORD(MODULE_MAP_FILE);
   RECORD(IMPORTS);
   RECORD(ORIGINAL_FILE);
-  RECORD(ORIGINAL_PCH_DIR);
   RECORD(ORIGINAL_FILE_ID);
   RECORD(INPUT_FILE_OFFSETS);
 
@@ -1187,7 +1186,7 @@
 
 /// Write the control block.
 void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
-  StringRef isysroot, StringRef OutputFile) {
+  StringRef isysroot) {
   using namespace llvm;
 
   Stream.EnterSubblock(CONTROL_BLOCK_ID, 5);
@@ -1470,21 +1469,6 @@
   Record.push_back(SM.getMainFileID().getOpaqueValue());
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  // Original PCH directory
-  if (!OutputFile.empty() && OutputFile != "-") {
-auto Abbrev = std::make_shared();
-Abbrev->Add(BitCodeAbbrevOp(ORIGINAL_PCH_DIR));
-Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // File name
-unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
-
-SmallString<128> OutputPath(OutputFile);
-PreparePathForOutput(OutputPath);
-StringRef origDir = llvm::sys::path::parent_path(OutputPath);
-
-RecordData::v

[PATCH] D131124: [Serialization] Remove `ORIGINAL_PCH_DIR` record

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6635f48e4aba: [Serialization] Remove `ORIGINAL_PCH_DIR` 
record (authored by akyrtzi).

Changed prior to commit:
  https://reviews.llvm.org/D131124?vs=449808&id=450439#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131124/new/

https://reviews.llvm.org/D131124

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/test/Modules/relative-original-dir.m
  clang/test/PCH/pch-output-path-independent.c

Index: clang/test/PCH/pch-output-path-independent.c
===
--- clang/test/PCH/pch-output-path-independent.c
+++ clang/test/PCH/pch-output-path-independent.c
@@ -1,6 +1,6 @@
 // RUN: rm -rf %t && mkdir -p %t/a %t/b
 
-// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch -fpcm-output-path-independent
-// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch -fpcm-output-path-independent
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/a/t1.pch
+// RUN: %clang_cc1 -triple x86_64-apple-macos11 -emit-pch %s -o %t/b/t2.pch
 
 // RUN: diff %t/a/t1.pch %t/b/t2.pch
Index: clang/test/Modules/relative-original-dir.m
===
--- clang/test/Modules/relative-original-dir.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t/normal-module-map
-// RUN: mkdir -p %t
-// RUN: cp -r %S/Inputs/normal-module-map %t
-// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA -emit-module %t/normal-module-map/module.map -o %t/normal-module-map/outdir/mod.pcm
-// RUN: llvm-bcanalyzer --dump --disable-histogram %t/normal-module-map/outdir/mod.pcm | FileCheck %s
-
-// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -25,14 +25,13 @@
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
 bool AllowASTWithErrors, bool IncludeTimestamps,
-bool ShouldCacheASTInMemory, bool OutputPathIndependent)
+bool ShouldCacheASTInMemory)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
   AllowASTWithErrors(AllowASTWithErrors),
-  ShouldCacheASTInMemory(ShouldCacheASTInMemory),
-  OutputPathIndependent(OutputPathIndependent) {
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
   this->Buffer->IsComplete = false;
 }
 
@@ -71,7 +70,7 @@
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
   PP.getDiagnostics().hasUncompilableErrorOccurred(),
-  ShouldCacheASTInMemory, OutputPathIndependent);
+  ShouldCacheASTInMemory);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -786,7 +786,6 @@
   RECORD(MODULE_MAP_FILE);
   RECORD(IMPORTS);
   RECORD(ORIGINAL_FILE);
-  RECORD(ORIGINAL_PCH_DIR);
   RECORD(ORIGINAL_FILE_ID);
   RECORD(INPUT_FILE_OFFSETS);
 
@@ -1187,7 +1186,7 @@
 
 /// Write the control block.
 void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
-  StringRef isysroot, StringRef OutputFile) {
+  StringRef isysroot) {
   using namespace llvm;
 
   Stream.EnterSubblock(CONTROL_BLOCK_ID, 5);
@@ -1470,21 +1469,6 @@
   Record.push_back(SM.getMainFileID().getOpaqueValue());
   Stream.EmitRecord(ORIGINAL_FILE_ID, Record);
 
-  // Original PCH directory
-  if (!OutputFile.empty() && OutputFile != "-") {
-auto Abbrev = std::make_shared();
-Abbrev->Add(BitCodeAbbrevOp(ORIGINAL_PCH_DIR));
-Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // File name
-unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
-
-SmallString<128> OutputPath(OutputFile);
-PreparePathForOutput(OutputPath);
-StringRef origDir = llvm::sys::path::parent_path(OutputPath);
-
-RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
-Stream.EmitRecordWithBlob(AbbrevCode, Record, origDir);
-  }
-
   std::set AffectingModuleMa

[PATCH] D102614: [index] Add support for type of pointers to class members

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

> What do you think about this patch? Can it be landed now? Or I should debug 
> the crash in the Windows version detected with the previous version of my 
> patch.

Is your change introducing the crash or is the crash triggered with the test 
file without your changes as well?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102614/new/

https://reviews.llvm.org/D102614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124558: [Tooling/DependencyScanning] Make skipping excluded PP ranges during dependency scanning the default

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi created this revision.
Herald added a project: All.
akyrtzi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is to improve maintenance a bit and remove need to maintain the additional 
option and related code-paths.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124558

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -191,14 +191,6 @@
 llvm::cl::desc("Reuse the file manager and its cache between invocations."),
 llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
 
-llvm::cl::opt SkipExcludedPPRanges(
-"skip-excluded-pp-ranges",
-llvm::cl::desc(
-"Use the preprocessor optimization that skips excluded conditionals by "
-"bumping the buffer pointer in the lexer instead of lexing the tokens  "
-"until reaching the end directive."),
-llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
-
 llvm::cl::opt ModuleName(
 "module-name", llvm::cl::Optional,
 llvm::cl::desc("the module of which the dependencies are to be computed"),
@@ -522,7 +514,7 @@
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
-SkipExcludedPPRanges, OptimizeArgs);
+OptimizeArgs);
   llvm::ThreadPool Pool(llvm::hardware_concurrency(NumThreads));
   std::vector> WorkerTools;
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
Index: clang/test/ClangScanDeps/regular_cdb.cpp
===
--- clang/test/ClangScanDeps/regular_cdb.cpp
+++ clang/test/ClangScanDeps/regular_cdb.cpp
@@ -20,11 +20,6 @@
 // RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-//
 // Make sure we didn't produce any dependency files!
 // RUN: not cat %t.dir/regular_cdb.d
 // RUN: not cat %t.dir/regular_cdb_clangcl.d
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -288,9 +288,7 @@
   OverlayFS->pushOverlay(InMemoryFS);
   RealFS = OverlayFS;
 
-  if (Service.canSkipExcludedPPRanges())
-PPSkipMappings =
-std::make_unique();
+  PPSkipMappings = std::make_unique();
   if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
 DepFS = new DependencyScanningWorkerFilesystem(
 Service.getSharedCache(), RealFS, PPSkipMappings.get());
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
@@ -15,9 +15,9 @@
 
 DependencyScanningService::DependencyScanningService(
 ScanningMode Mode, ScanningOutputFormat Format, bool ReuseFileManager,
-bool SkipExcludedPPRanges, bool OptimizeArgs)
+bool OptimizeArgs)
 : Mode(Mode), Format(Format), ReuseFileManager(ReuseFileManager),
-  SkipExcludedPPRanges(SkipExcludedPPRanges), OptimizeArgs(OptimizeArgs) {
+  OptimizeArgs(OptimizeArgs) {
   // Initialize targets for object file support.
   llvm::InitializeAllTargets();
   llvm::InitializeAllTargetMCs();
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
===
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
@@ -48,7 +48,6 @@
 public:
   DependencyScanningService(ScanningMode Mode, ScanningOutputFormat Format,
 bool ReuseFileManager = true,
-bool SkipExcludedPPRanges = true,
  

[PATCH] D124558: [Tooling/DependencyScanning] Make skipping excluded PP ranges during dependency scanning the default

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi updated this revision to Diff 425833.
akyrtzi added a comment.

Change APIs to accept a reference of `ExcludedPreprocessorDirectiveSkipMapping` 
instead of a pointer, since it is required now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124558/new/

https://reviews.llvm.org/D124558

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- clang/unittests/Tooling/DependencyScannerTest.cpp
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -212,8 +212,8 @@
 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.enableMinimizationOfAllFiles(); // Let's be explicit for clarity.
   auto StatusMinimized0 = DepFS.status("/mod.h");
@@ -235,8 +235,8 @@
 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.disableMinimization("/mod.h");
   auto StatusFull0 = DepFS.status("/mod.h");
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -191,14 +191,6 @@
 llvm::cl::desc("Reuse the file manager and its cache between invocations."),
 llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
 
-llvm::cl::opt SkipExcludedPPRanges(
-"skip-excluded-pp-ranges",
-llvm::cl::desc(
-"Use the preprocessor optimization that skips excluded conditionals by "
-"bumping the buffer pointer in the lexer instead of lexing the tokens  "
-"until reaching the end directive."),
-llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
-
 llvm::cl::opt ModuleName(
 "module-name", llvm::cl::Optional,
 llvm::cl::desc("the module of which the dependencies are to be computed"),
@@ -522,7 +514,7 @@
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
-SkipExcludedPPRanges, OptimizeArgs);
+OptimizeArgs);
   llvm::ThreadPool Pool(llvm::hardware_concurrency(NumThreads));
   std::vector> WorkerTools;
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
Index: clang/test/ClangScanDeps/regular_cdb.cpp
===
--- clang/test/ClangScanDeps/regular_cdb.cpp
+++ clang/test/ClangScanDeps/regular_cdb.cpp
@@ -20,11 +20,6 @@
 // RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-//
 // Make sure we didn't produce any dependency files!
 // RUN: not cat %t.dir/regular_cdb.d
 // RUN: not cat %t.dir/regular_cdb_clangcl.d
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -137,7 +137,7 @@
   DependencyScanningAction(
   StringRef WorkingDirectory, DependencyConsumer &Consumer,
   llvm::IntrusiveRefCntPtr DepFS,
-  ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings,
+  ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings,
  

[PATCH] D124558: [Tooling/DependencyScanning] Make skipping excluded PP ranges during dependency scanning the default

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

@jansvoboda11 thanks for reviewing! I've changed APIs to use a reference 
instead of a pointer and removed the unnecessary check and heap allocations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124558/new/

https://reviews.llvm.org/D124558

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124558: [Tooling/DependencyScanning] Make skipping excluded PP ranges during dependency scanning the default

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG42823beb1d71: [Tooling/DependencyScanning] Make skipping 
excluded PP ranges during dependency… (authored by akyrtzi).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124558/new/

https://reviews.llvm.org/D124558

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/unittests/Tooling/DependencyScannerTest.cpp

Index: clang/unittests/Tooling/DependencyScannerTest.cpp
===
--- clang/unittests/Tooling/DependencyScannerTest.cpp
+++ clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -212,8 +212,8 @@
 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.enableMinimizationOfAllFiles(); // Let's be explicit for clarity.
   auto StatusMinimized0 = DepFS.status("/mod.h");
@@ -235,8 +235,8 @@
 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.disableMinimization("/mod.h");
   auto StatusFull0 = DepFS.status("/mod.h");
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -191,14 +191,6 @@
 llvm::cl::desc("Reuse the file manager and its cache between invocations."),
 llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
 
-llvm::cl::opt SkipExcludedPPRanges(
-"skip-excluded-pp-ranges",
-llvm::cl::desc(
-"Use the preprocessor optimization that skips excluded conditionals by "
-"bumping the buffer pointer in the lexer instead of lexing the tokens  "
-"until reaching the end directive."),
-llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
-
 llvm::cl::opt ModuleName(
 "module-name", llvm::cl::Optional,
 llvm::cl::desc("the module of which the dependencies are to be computed"),
@@ -522,7 +514,7 @@
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
-SkipExcludedPPRanges, OptimizeArgs);
+OptimizeArgs);
   llvm::ThreadPool Pool(llvm::hardware_concurrency(NumThreads));
   std::vector> WorkerTools;
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
Index: clang/test/ClangScanDeps/regular_cdb.cpp
===
--- clang/test/ClangScanDeps/regular_cdb.cpp
+++ clang/test/ClangScanDeps/regular_cdb.cpp
@@ -20,11 +20,6 @@
 // RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-//
 // Make sure we didn't produce any dependency files!
 // RUN: not cat %t.dir/regular_cdb.d
 // RUN: not cat %t.dir/regular_cdb_clangcl.d
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -137,7 +137,7 @@
   DependencyScanningAction(
   StringRef WorkingDirectory, DependencyConsumer &Consumer,
   llvm::IntrusiveRefCntPtr DepFS,
-  ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings,
+  ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappin

[PATCH] D124687: [Tooling/DependencyScanning & Preprocessor] Refactor dependency scanning to record and use pre-lexed preprocessor directive tokens, instead of minimized sources

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/include/clang/Lex/PreprocessorOptions.h:214
+  FileEntryRef)>
+  DependencyDirectivesForFile;
 

jansvoboda11 wrote:
> To be honest, I'm not a fan of using `PreprocessorOptions` to carry state 
> between compiler invocations.
> 
> Could we implement a different mechanism for this in a prep patch an put 
> `DependencyDirectivesForFile` there? `FailedModules` also seem as a state 
> rather than options.
Would you be ok to consider this as a task for follow-up? It seems orthogonal 
to the intended changes of this patch (I mean the patch doesn't make things 
worse in that respect AFAICT)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124687/new/

https://reviews.llvm.org/D124687

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124687: [Tooling/DependencyScanning & Preprocessor] Refactor dependency scanning to record and use pre-lexed preprocessor directive tokens, instead of minimized sources

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D124687#3485710 , @tschuett wrote:

> Could you split this into smaller patches?

I'll split up the renames to a separate patch so that it is easier to see the 
code that affects functionality. Not sure if it can be broken further, after 
that the changes are interdependent.
Once I separate the renames we can see how it looks, I suspect it will be much 
easier to review.

> Does this support C++20 modules or is it limited to clang header modules?

Dependency scanning should work the same as before for C++20 modules, there 
should not be a change (either making it worse or better).

> Is there overhead in the non dependency scanning mode?

Good suggestion, I'll do some measurements and get back to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124687/new/

https://reviews.llvm.org/D124687

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124715: Use QoS class Utility for ThreadPriority::Low on Mac

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

My recommendation is that indexing belongs to 'utility', as @stefanhaller 
mentioned the user is actively depending on functionality coming from the index.
That said, you may want to consider dynamically switching to background if 
running on laptop with battery, or other heuristics, but that could be a 
follow-up enhancement, I don't think always using 'background' is appropriate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124715/new/

https://reviews.llvm.org/D124715

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124687: [Tooling/DependencyScanning & Preprocessor] Refactor dependency scanning to record and use pre-lexed preprocessor directive tokens, instead of minimized sources

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D124687#3486473 , @akyrtzi wrote:

> In D124687#3485710 , @tschuett 
> wrote:
>
>> Is there overhead in the non dependency scanning mode?
>
> Good suggestion, I'll do some measurements and get back to you.

I took measurements for preprocessing the clang sources, with a 
release+thin-LTO build, and differences are within the noise level.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124687/new/

https://reviews.llvm.org/D124687

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits