[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov added inline comments.



Comment at: clang/include/clang/AST/Type.h:1827-1830
+if (Dependent)
+  Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
+if (InstantiationDependent)
+  Deps |= TypeDependence::Instantiation;

sammccall wrote:
> AlexeySachkov wrote:
> > @ilya-biryukov, Is this code snippet correct?
> > 
> > It seems to be, that it should look like:
> > ```
> > if (Dependent)
> >   Deps |= TypeDependence::Dependent;
> > if (InstantiationDependent)
> >   Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
> > ```
> I agree that seems clearer, but ISTM they are equivalent because a dependent 
> type is always instantiation-dependent (right?)
> 
> Are you seeing related problems?
> Are you seeing related problems?

I though I was seeing a related problem, but it turned out that I wasn't.

Looking at the code again with a clear head, I believe that everything is 
correct here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/include/clang/AST/Type.h:1827-1830
+if (Dependent)
+  Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
+if (InstantiationDependent)
+  Deps |= TypeDependence::Instantiation;

AlexeySachkov wrote:
> @ilya-biryukov, Is this code snippet correct?
> 
> It seems to be, that it should look like:
> ```
> if (Dependent)
>   Deps |= TypeDependence::Dependent;
> if (InstantiationDependent)
>   Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
> ```
I agree that seems clearer, but ISTM they are equivalent because a dependent 
type is always instantiation-dependent (right?)

Are you seeing related problems?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov added inline comments.



Comment at: clang/include/clang/AST/Type.h:1827-1830
+if (Dependent)
+  Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
+if (InstantiationDependent)
+  Deps |= TypeDependence::Instantiation;

@ilya-biryukov, Is this code snippet correct?

It seems to be, that it should look like:
```
if (Dependent)
  Deps |= TypeDependence::Dependent;
if (InstantiationDependent)
  Deps |= TypeDependence::Dependent | TypeDependence::Instantiation;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rGec3060c72de6: [AST] Refactor propagation of dependency bits. 
NFC (authored by ilya-biryukov, committed by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D71920?vs=247905=248134#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprConcepts.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConcepts.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependence(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -25,6 +25,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12719,9 +12720,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependence(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -11,8 +11,10 @@
 //===--===//
 
 #include "clang/AST/TemplateName.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/lib/AST/TemplateName.cpp:208
  "overloaded templates shouldn't survive to here");
+  D |= TemplateNameDependence::DependentInstantiation;
+  return D;

hokein wrote:
> sammccall wrote:
> > what's this line about?
> this indicates that the template name is dependent if it doesn't refer to any 
> known template declarations (getAsTemplateDecl() returns null). removing it 
> will cause a test failure.
oh, I missed the early return in the other case.
nit: I'd suggest a single return and put this in an else { ... }, which I think 
makes it slightly clearer that the two "parts" of this function are simply 
composed together. Up to you though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/AST/TemplateName.cpp:208
  "overloaded templates shouldn't survive to here");
+  D |= TemplateNameDependence::DependentInstantiation;
+  return D;

sammccall wrote:
> what's this line about?
this indicates that the template name is dependent if it doesn't refer to any 
known template declarations (getAsTemplateDecl() returns null). removing it 
will cause a test failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 247905.
hokein marked an inline comment as done.
hokein added a comment.

move the assert to overloadedTemplate switch case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprConcepts.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConcepts.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependence(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -25,6 +25,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12719,9 +12720,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependence(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -11,8 +11,10 @@
 //===--===//
 
 #include "clang/AST/TemplateName.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +170,54 @@
   return TemplateName(Decl);
 }
 
-bool TemplateName::isDependent() const {

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

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

thanks, that looks better to me




Comment at: clang/lib/AST/TemplateName.cpp:193
+  } else {
+assert(!getAsOverloadedTemplate() &&
+   "overloaded templates shouldn't survive to here");

hokein wrote:
> sammccall wrote:
> > As far as I can tell, an overloaded template will always return null for 
> > getAsTemplateDecl(). So this assert can be hoisted to the top, which I 
> > think would be clearer.
> I didn't get your point here, did you mean moving the assert to the `switch 
> (getKind())` like 
> 
> ```
> switch (getKind()) {
>   case OverloadedTemplate:
> assert(!GetAsTemplateDecl());
> }
> ```
> ?
ITYM `assert(getAsTemplateDecl())`

Yes, but also if this is an `OverloadedTemplate`, then getAsTemplateDecl is 
always false. So this can just be `assert(false && "some message")` in that 
case statement



Comment at: clang/lib/AST/TemplateName.cpp:208
  "overloaded templates shouldn't survive to here");
+  D |= TemplateNameDependence::DependentInstantiation;
+  return D;

what's this line about?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 247881.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprConcepts.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConcepts.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependence(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -25,6 +25,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12719,9 +12720,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependence(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -11,8 +11,10 @@
 //===--===//
 
 #include "clang/AST/TemplateName.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +170,55 @@
   return TemplateName(Decl);
 }
 
-bool TemplateName::isDependent() const {
+TemplateNameDependence 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/AST/TemplateName.cpp:193
+  } else {
+assert(!getAsOverloadedTemplate() &&
+   "overloaded templates shouldn't survive to here");

sammccall wrote:
> As far as I can tell, an overloaded template will always return null for 
> getAsTemplateDecl(). So this assert can be hoisted to the top, which I think 
> would be clearer.
I didn't get your point here, did you mean moving the assert to the `switch 
(getKind())` like 

```
switch (getKind()) {
  case OverloadedTemplate:
assert(!GetAsTemplateDecl());
}
```
?



Comment at: clang/lib/AST/TemplateName.cpp:205
+  // Dependent template name is always instantiation dependent.
+  if (Dependency & TemplateNameDependence::Dependent)
+Dependency |= TemplateNameDependence::DependentInstantiation;

sammccall wrote:
> why not just add DependentInstantiation above in the first place?
> Tracking incorrect bits and then fixing them at the end seems a bit confusing.
oops, I was confused by the `DependentInstantiation` and `Dependent` bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/lib/AST/TemplateName.cpp:174
+TemplateNameDependence TemplateName::getDependence() const {
+  auto Dependency = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName())

nit: please don't use "Dependency" to mean "Dependence" :-)

I find this easier to read with a short name here (like `D` or the previous 
`F`, but up to you.



Comment at: clang/lib/AST/TemplateName.cpp:175
+  auto Dependency = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName())
+Dependency |=

I think it would add some useful structure to put everything except the 
getAsTemplateDecl() in a `switch (getKind())` rather than ifs scattered around. 
This would make it clearer which cases are disjoint.



Comment at: clang/lib/AST/TemplateName.cpp:193
+  } else {
+assert(!getAsOverloadedTemplate() &&
+   "overloaded templates shouldn't survive to here");

As far as I can tell, an overloaded template will always return null for 
getAsTemplateDecl(). So this assert can be hoisted to the top, which I think 
would be clearer.



Comment at: clang/lib/AST/TemplateName.cpp:201
+DTN->getQualifier()->containsUnexpandedParameterPack())
+  Dependency |= TemplateNameDependence::UnexpandedPack;
+  if (getAsSubstTemplateTemplateParmPack())

Why are we querying/propagating individual bits here rather than converting and 
adding DTN->getQualifier()->getDependence()? Are we deliberately dropping some 
of the bits?

(Checking individual bits makes adding new bits e.g. errors harder, as they 
won't be propagated by default)



Comment at: clang/lib/AST/TemplateName.cpp:205
+  // Dependent template name is always instantiation dependent.
+  if (Dependency & TemplateNameDependence::Dependent)
+Dependency |= TemplateNameDependence::DependentInstantiation;

why not just add DependentInstantiation above in the first place?
Tracking incorrect bits and then fixing them at the end seems a bit confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 247818.
hokein added a comment.

- rebase to master
- address my comments
- added a FIXME about the RequiresExpr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprConcepts.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConcepts.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependence(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -25,6 +25,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12719,9 +12720,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependence(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -11,8 +11,10 @@
 //===--===//
 
 #include "clang/AST/TemplateName.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +170,53 @@
   return TemplateName(Decl);
 }
 
-bool TemplateName::isDependent() const {
+TemplateNameDependence 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein commandeered this revision.
hokein edited reviewers, added: sammccall; removed: hokein.
hokein added inline comments.



Comment at: clang/lib/AST/TemplateName.cpp:173
+TemplateNameDependence TemplateName::getDependence() const {
+  auto F = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName()) {

hokein wrote:
> This part of refactoring seems a little scary to me,  I think it is correct 
> by comparing with the previous version. but the getDependence() now is very 
> **complicated**, I have no idea what it is doing.
> 
> instead of merging three different non-trivial if branches into a single 
> function, maybe keep them as it-is.
I have restructured the code, let me know whether it is clear now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/include/clang/AST/DependencyFlags.h:57
+  struct NAME##Scope { 
\
+enum NAME {
\
+  UnexpandedPack = 1,  
\

should we make the enum as `uint8_t`  as the other `ExprDependence` above?



Comment at: clang/include/clang/AST/DependencyFlags.h:81
+  auto E =
+  static_cast(TA & ~TemplateArgumentDependence::Dependent);
+  if (TA & TemplateArgumentDependence::Dependent)

nit: I think we can get rid of the `static_cast`, sine we already use 
`LLVM_MARK_AS_BITMASK_ENUM`.



Comment at: clang/include/clang/AST/Type.h:1468
 
-/// Whether this type is a dependent type (C++ [temp.dep.type]).
-unsigned Dependent : 1;

would be nice to keep this comments in the new `TypeDependence` struct.



Comment at: clang/lib/AST/TemplateName.cpp:173
+TemplateNameDependence TemplateName::getDependence() const {
+  auto F = TemplateNameDependence::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName()) {

This part of refactoring seems a little scary to me,  I think it is correct by 
comparing with the previous version. but the getDependence() now is very 
**complicated**, I have no idea what it is doing.

instead of merging three different non-trivial if branches into a single 
function, maybe keep them as it-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

Thanks Richard! Haojian agreed to take a look at this.




Comment at: clang/lib/AST/ExprConcepts.cpp:186
+  if (Dependent)
+addDependence(ExprDependence::ValueInstantiation);
 }

Per Richard's comment, it sounds like this may be incorrect in obscure cases 
(if we have a non-dependent requirement that is always unsatisfied, but 
parameters are instantiation-dependent). I'll add a FIXME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D71920#1870195 , @sammccall wrote:

> If you'd like to have `concept::Requirement` use a similar bitfield, I'd just 
> like to confirm my understanding of the current code before refactoring it:
>
> - there's just one `Dependent` bit (along with `UnexpandedPack`) - 
> instantiation-dependence isn't relevant?


Correct; requirements are weird in this regard because substitution failure 
affects their value rather than their validity, so there's no difference 
between "refers to template parameter / substitution can fail" and "value 
depends on template parameters"

> - RequiresExpr is only instantiation-dependent if value-dependent (unlike 
> other exprs)

I don't think that's quite correct: a RequiresExpr should also be 
instantiation-dependent if it has instantiation-dependent parameters. Other 
than that case, it's both value-dependent and instantiation-dependent if it has 
dependent requirements and neither otherwise, so yes, the two flags are the 
same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D71920#1884740 , @sammccall wrote:

> @rsmith Friendly ping. Do you want to review this/other patches in this 
> sequence, or should we be a bit bolder and pull you in when we're uncertain 
> in principle what to do?


I'm fine with you making progress on this without my involvement; I consider 
the general plan described in https://reviews.llvm.org/D65591 to be approved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

@rsmith Friendly ping. Do you want to review this/other patches in this 
sequence, or should we be a bit bolder and pull you in when we're uncertain in 
principle what to do?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall commandeered this revision.
sammccall added a reviewer: ilya-biryukov.
sammccall added a comment.

@ilya-biryukov isn't working on clang fulltime anymore :-(

I'm going to try to pick up the work on RecoveryExpr, starting with this patch.

@rsmith I think this is waiting on your review (sorry if I've missed anything 
else that was outstanding).

If you'd like to have `concept::Requirement` use a similar bitfield, I'd just 
like to confirm my understanding of the current code before refactoring it:

- there's just one `Dependent` bit (along with `UnexpandedPack`) - 
instantiation-dependence isn't relevant?
- RequiresExpr is only instantiation-dependent if value-dependent (unlike other 
exprs)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 243915.
sammccall added a comment.

Rebase (ConceptSpecializationExpr moved).
Added FIXME to use enum for concept requirement dependence, it's still murky to 
me.
addDependencies -> addDependence, for clarity.
Move variably-modified into TypeDependence bitfield.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprConcepts.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConcepts.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependence(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -24,6 +24,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12718,9 +12719,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependence(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +169,48 @@
   return TemplateName(Decl);
 }
 
-bool 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61841 tests passed, 5 failed 
and 780 were skipped.

  failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

{icon times-circle color=red} clang-tidy: fail. clang-tidy found 0 errors and 4 
warnings 
.
 0 of them are added as review comments below (why? 
).

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I've opted for duplicating the common flags across all the introduced enums 
(contains-unexpanded-pack, instantiation-dependent) , this is somewhat ugly, 
but everything else is even more complicated to use.
Less enums would also be a good idea probably, see the relevant comment.

Other than that, this should be ready for the next round.

In D71920#1843488 , @rsmith wrote:

> I don't like the name `getDependencies`, because the function is not getting 
> a list of dependencies, it's getting flags that indicate whether certain 
> properties of the construct are dependent. Maybe `getDependence` or 
> `getDependenceFlags` would be a better name? Likewise, instead of 
> `addDependencies`, perhaps `addDependence`?


Good point. I've opted for `getDependence`, but didn't have enough time today 
to change `addDependencies`, will make sure to do it in the next iteration.




Comment at: clang/include/clang/AST/DependencyFlags.h:17-28
+enum class DependencyFlags : uint8_t {
+  Type = 1,
+  Value = 2,
+  Instantiation = 4,
+  UnexpandedPack = 8,
+
+  // Shorthands for commonly used combinations.

ilya-biryukov wrote:
> rsmith wrote:
> > Hmm. We have a different set of propagated flags for types (dependent / 
> > instantiation dependent / unexpanded pack / variably-modified) and for (eg) 
> > template arguments and nested name specifiers (dependent / instantiation 
> > dependent / unexpanded pack). It would be nice to use the same machinery 
> > everywhere without introducing the possibility of meaningless states and 
> > giving the wrong names to some states.
> > 
> > I think we should aim for something more type-safe than this: use a 
> > different type for each different family of AST nodes, so we don't conflate 
> > "dependent" for template arguments with one or both of "type-dependent" and 
> > "value-dependent" for expressions, which mean different things.
> Yep, sounds good, probably the types would end up being more complicated, but 
> I also like more type-safe approach.
> Let me try to come up with something, I'll send a patch today.
I've introduced a separate enum for each of the AST classes, but that looks 
like an overkill.
We could probably merge the three for template arguments, template names and 
nested name specifiers into one.
(Would allow to get rid of macros too).

Note that `TypeDependence` is missing variably-modified bit, was't sure if it's 
part of the dependency propagation (don't have enough context to understand 
what it does and didn't look into it). From you message, I assume you would 
prefer to have it in the enum too?

WDYT about the approach in general?



Comment at: clang/include/clang/AST/DependencyFlags.h:32-59
+constexpr inline DependencyFlags operator~(DependencyFlags F) {
+  return static_cast(
+  ~static_cast(F) & static_cast(DependencyFlags::All));
+}
+
+constexpr inline DependencyFlags operator|(DependencyFlags L,
+   DependencyFlags R) {

rsmith wrote:
> You can use LLVM's `BitmaskEnum` mechanism in place of these operators. 
> (`#include "clang/Basic/BitmaskEnum.h"` and add 
> `LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/UnexpandedPack)` to the end of 
> the enum.)
Thanks, that does look better. I've used this and also switched to 
non-strongly-typed enums to allow simple conversions to bool (and code like 
`bool IsTypeDependent = D & ExprDependence::Type`).

There's a catch, though. We can't use `LLVM_MARK_AS_BITMASK_ENUM` on two enums 
inside the same namespace, it results in redeclaration of the same enumerator.

So I had to put them into structs (to introduce a scope). Might be a bit weird, 
let me know what you think, changing back to strongly-typed enums should not be 
too hard. That would mean we need helpers like `isTypeDependent` or checks like 
`(D & Flags::Type) != Flags::None`, both don't look good.



Comment at: clang/include/clang/AST/DependencyFlags.h:81-85
+inline DependencyFlags turnTypeToValueDependency(DependencyFlags F) {
+  if (!isTypeDependent(F))
+return F;
+  return (F & ~DependencyFlags::Type) | DependencyFlags::Value;
+}

ilya-biryukov wrote:
> rsmith wrote:
> > This whole function should be equivalent to just `F & 
> > ~DependencyFlags::Type`. Any type-dependent expression must also be 
> > value-dependent, so you should never need to set the `::Value` bit. Perhaps 
> > we could assert this somewhere.
> Good point, I'll try to find a place to assert this with multiple types for 
> different AST categories.
Was thinking about the best place for assert.
Putting it into headers means introducing possibly ODR violations, we do care 
about keeping those off LLVM, right?
Putting it into the .cpp files means we're loosing optimization opportunities 
in non-LTO builds (those functions should really be inlined).

Decided to not put the 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 240958.
ilya-biryukov marked an inline comment as done and an inline comment as not 
done.
ilya-biryukov added a comment.

- Use different types for different AST categories
- Rename to getDependence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -11,7 +11,6 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/AttrIterator.h"
@@ -22,6 +21,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -49,6 +49,7 @@
 #include "clang/Basic/TypeTraits.h"
 #include "clang/Lex/Token.h"
 #include "clang/Serialization/ASTBitCodes.h"
+#include "clang/Serialization/ASTRecordReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
@@ -511,10 +512,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  auto Deps = ExprDependence::None;
+  if (TypeDependent)
+Deps |= ExprDependence::Type;
+  if (ValueDependent)
+Deps |= ExprDependence::Value;
+  if (InstantiationDependent)
+Deps |= ExprDependence::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= ExprDependence::UnexpandedPack;
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "clang/Sema/Overload.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
@@ -24,6 +24,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
@@ -12694,9 +12695,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependencies(ExprDependence::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +169,48 @@
   return TemplateName(Decl);
 }
 
-bool TemplateName::isDependent() const {
+TemplateNameDependence 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang/include/clang/AST/DependencyFlags.h:17-28
+enum class DependencyFlags : uint8_t {
+  Type = 1,
+  Value = 2,
+  Instantiation = 4,
+  UnexpandedPack = 8,
+
+  // Shorthands for commonly used combinations.

rsmith wrote:
> Hmm. We have a different set of propagated flags for types (dependent / 
> instantiation dependent / unexpanded pack / variably-modified) and for (eg) 
> template arguments and nested name specifiers (dependent / instantiation 
> dependent / unexpanded pack). It would be nice to use the same machinery 
> everywhere without introducing the possibility of meaningless states and 
> giving the wrong names to some states.
> 
> I think we should aim for something more type-safe than this: use a different 
> type for each different family of AST nodes, so we don't conflate "dependent" 
> for template arguments with one or both of "type-dependent" and 
> "value-dependent" for expressions, which mean different things.
Yep, sounds good, probably the types would end up being more complicated, but I 
also like more type-safe approach.
Let me try to come up with something, I'll send a patch today.



Comment at: clang/include/clang/AST/DependencyFlags.h:81-85
+inline DependencyFlags turnTypeToValueDependency(DependencyFlags F) {
+  if (!isTypeDependent(F))
+return F;
+  return (F & ~DependencyFlags::Type) | DependencyFlags::Value;
+}

rsmith wrote:
> This whole function should be equivalent to just `F & 
> ~DependencyFlags::Type`. Any type-dependent expression must also be 
> value-dependent, so you should never need to set the `::Value` bit. Perhaps 
> we could assert this somewhere.
Good point, I'll try to find a place to assert this with multiple types for 
different AST categories.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I don't like the name `getDependencies`, because the function is not getting a 
list of dependencies, it's getting flags that indicate whether certain 
properties of the construct are dependent. Maybe `getDependence` or 
`getDependenceFlags` would be a better name? Likewise, instead of 
`addDependencies`, perhaps `addDependence`?




Comment at: clang/include/clang/AST/DependencyFlags.h:17-28
+enum class DependencyFlags : uint8_t {
+  Type = 1,
+  Value = 2,
+  Instantiation = 4,
+  UnexpandedPack = 8,
+
+  // Shorthands for commonly used combinations.

Hmm. We have a different set of propagated flags for types (dependent / 
instantiation dependent / unexpanded pack / variably-modified) and for (eg) 
template arguments and nested name specifiers (dependent / instantiation 
dependent / unexpanded pack). It would be nice to use the same machinery 
everywhere without introducing the possibility of meaningless states and giving 
the wrong names to some states.

I think we should aim for something more type-safe than this: use a different 
type for each different family of AST nodes, so we don't conflate "dependent" 
for template arguments with one or both of "type-dependent" and 
"value-dependent" for expressions, which mean different things.



Comment at: clang/include/clang/AST/DependencyFlags.h:32-59
+constexpr inline DependencyFlags operator~(DependencyFlags F) {
+  return static_cast(
+  ~static_cast(F) & static_cast(DependencyFlags::All));
+}
+
+constexpr inline DependencyFlags operator|(DependencyFlags L,
+   DependencyFlags R) {

You can use LLVM's `BitmaskEnum` mechanism in place of these operators. 
(`#include "clang/Basic/BitmaskEnum.h"` and add 
`LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/UnexpandedPack)` to the end of the 
enum.)



Comment at: clang/include/clang/AST/DependencyFlags.h:81-85
+inline DependencyFlags turnTypeToValueDependency(DependencyFlags F) {
+  if (!isTypeDependent(F))
+return F;
+  return (F & ~DependencyFlags::Type) | DependencyFlags::Value;
+}

This whole function should be equivalent to just `F & ~DependencyFlags::Type`. 
Any type-dependent expression must also be value-dependent, so you should never 
need to set the `::Value` bit. Perhaps we could assert this somewhere.



Comment at: clang/lib/AST/NestedNameSpecifier.cpp:221
   if (Base.getType()->isDependentType())
-return true;
+return DependencyFlags::Type;
 

This is wrong: `super` should be instantiation-dependent whenever `Specifier` 
names a dependent type. Please add a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61841 tests passed, 5 failed 
and 780 were skipped.

  failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 240604.
ilya-biryukov added a comment.

- Use DependencyFlags for TemplateName and NestedNameSpecifier


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -511,10 +511,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  DependencyFlags Deps = DependencyFlags::None;
+  if (TypeDependent)
+Deps |= DependencyFlags::Type;
+  if (ValueDependent)
+Deps |= DependencyFlags::Value;
+  if (InstantiationDependent)
+Deps |= DependencyFlags::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= DependencyFlags::UnexpandedPack;
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12694,9 +12694,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependencies(DependencyFlags::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DependencyFlags.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
@@ -168,52 +169,50 @@
   return TemplateName(Decl);
 }
 
-bool TemplateName::isDependent() const {
+DependencyFlags TemplateName::getDependencies() const {
+  DependencyFlags F = DependencyFlags::None;
+  if (QualifiedTemplateName *QTN = getAsQualifiedTemplateName()) {
+F |= turnNestedNameSpecifierToNameDependencies(
+QTN->getQualifier()->getDependencies());
+  } else if (DependentTemplateName *DTN = getAsDependentTemplateName()) {
+if (DTN->getQualifier())
+  F |= turnNestedNameSpecifierToNameDependencies(
+  DTN->getQualifier()->getDependencies());
+  }
+
   if (TemplateDecl *Template = getAsTemplateDecl()) {
-if (isa(Template))
-  return true;
+if (auto *TTP = dyn_cast(Template)) {
+  F |= DependencyFlags::TypeValueInstantiation;
+  if (TTP->isParameterPack())
+F |= DependencyFlags::UnexpandedPack;
+}
 // FIXME: Hack, getDeclContext() can be null if Template is still
 // initializing due to PCH reading, so we check it before using it.
 // Should probably modify TemplateSpecializationType to allow constructing
 // it without the isDependent() checking.
-return Template->getDeclContext() &&
-   Template->getDeclContext()->isDependentContext();
+if (Template->getDeclContext() &&
+Template->getDeclContext()->isDependentContext())
+  F |= DependencyFlags::TypeValueInstantiation;
+  } else {
+assert(!getAsOverloadedTemplate() &&
+   "overloaded templates shouldn't survive to here");
+F |= DependencyFlags::TypeValueInstantiation;
   }
+  if (getAsSubstTemplateTemplateParmPack() != nullptr)
+F |= DependencyFlags::UnexpandedPack;
+  return F;
+}
 
-  

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61745 tests passed, 0 failed 
and 780 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 237577.
ilya-biryukov added a comment.

- Add compound assignment operations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -511,10 +511,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  DependencyFlags Deps = DependencyFlags::None;
+  if (TypeDependent)
+Deps |= DependencyFlags::Type;
+  if (ValueDependent)
+Deps |= DependencyFlags::Value;
+  if (InstantiationDependent)
+Deps |= DependencyFlags::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps |= DependencyFlags::UnexpandedPack;
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12695,9 +12695,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependencies(DependencyFlags::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -111,84 +111,65 @@
   return TemplateArgument(Args.copy(Context));
 }
 
-bool TemplateArgument::isDependent() const {
+DependencyFlags TemplateArgument::getDependencies() const {
+  DependencyFlags Deps = DependencyFlags::None;
   switch (getKind()) {
   case Null:
 llvm_unreachable("Should not have a NULL template argument");
 
   case Type:
-return getAsType()->isDependentType() ||
-   isa(getAsType());
+Deps = getAsType()->getDependencies();
+if (isa(getAsType()))
+  Deps |= DependencyFlags::Type;
+return Deps;
 
   case Template:
-return getAsTemplate().isDependent();
+// FIXME: add TemplateName::getDependencies.
+if (getAsTemplate().isDependent())
+  Deps |= DependencyFlags::TypeValue;
+if (getAsTemplate().isInstantiationDependent())
+  Deps |= DependencyFlags::Instantiation;
+if (getAsTemplate().containsUnexpandedParameterPack())
+  Deps |= DependencyFlags::UnexpandedPack;
+return Deps;
 
   case TemplateExpansion:
-return true;
+return DependencyFlags::TypeValueInstantiation;
 
-  case Declaration:
-if (DeclContext *DC = dyn_cast(getAsDecl()))
-  return DC->isDependentContext();
-return getAsDecl()->getDeclContext()->isDependentContext();
+  case Declaration: {
+auto *DC = dyn_cast(getAsDecl());
+if (!DC)
+  DC = getAsDecl()->getDeclContext();
+if (DC->isDependentContext())
+  Deps = DependencyFlags::TypeValueInstantiation;
+return Deps;
+  }
 
   case NullPtr:
-return false;
-
   case Integral:
-// Never dependent
-return false;
+return DependencyFlags::None;
 
   case Expression:
-return (getAsExpr()->isTypeDependent() || getAsExpr()->isValueDependent() ||
-isa(getAsExpr()));
+Deps = getAsExpr()->getDependencies();
+if (isa(getAsExpr()))
+  Deps |= DependencyFlags::TypeValueInstantiation;
+return Deps;
 
   case Pack:
 for (const auto  : pack_elements())
-  if (P.isDependent())
-return true;
-return false;
+  Deps |= P.getDependencies();
+return Deps;
   }
+  llvm_unreachable("unhandled ArgKind");
+}
 
-  llvm_unreachable("Invalid 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@rsmith, gentle ping. WDYT? Is this a step in the right direction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clang/include/clang/AST/Expr.h:126
+if (TD)
+  D = D | DependencyFlags::Type;
+if (VD)

riccibruno wrote:
> ilya-biryukov wrote:
> > Mordante wrote:
> > > Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D 
> > > |= DependencyFlags::Type;` ? The latter seems to be more common.
> > Would also prefer `D |=`, but it leads to compilation errors.
> > 
> > The builtin `operator |=` accepts ints and, therefore, fails on 
> > strongly-typed enums.
> > And, AFAIK, there's no way to redefine `operator |=` for non-class types.
> You certainly can define a compound assignment operator for an enumeration 
> type. It is only non-compound-assignment that is special cased and required 
> to be a member function.
> 
> Example: https://godbolt.org/z/JV5uPw
Ah, thanks! So it turns out I was wrong. Will update the patch.



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:530
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));

riccibruno wrote:
> I think it would be nicer to add an abbreviation for the right number of bits 
> (using `DependencyFlagsBits`) and as you say just serialize/deserialize all 
> the flags in one go.
Will do this in a follow-up.
That would be a functional change, I'm aiming to keep this one an NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/include/clang/AST/Expr.h:126
+if (TD)
+  D = D | DependencyFlags::Type;
+if (VD)

ilya-biryukov wrote:
> Mordante wrote:
> > Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D |= 
> > DependencyFlags::Type;` ? The latter seems to be more common.
> Would also prefer `D |=`, but it leads to compilation errors.
> 
> The builtin `operator |=` accepts ints and, therefore, fails on 
> strongly-typed enums.
> And, AFAIK, there's no way to redefine `operator |=` for non-class types.
You certainly can define a compound assignment operator for an enumeration 
type. It is only non-compound-assignment that is special cased and required to 
be a member function.

Example: https://godbolt.org/z/JV5uPw



Comment at: clang/include/clang/AST/Expr.h:134
+
+ExprBits.Dependent = static_cast(D);
 ExprBits.ValueKind = VK;

You may want to add an assertion here to make sure that no truncation occurred.



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:530
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));

I think it would be nicer to add an abbreviation for the right number of bits 
(using `DependencyFlagsBits`) and as you say just serialize/deserialize all the 
flags in one go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61158 tests passed, 0 failed 
and 728 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 4 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clang/include/clang/AST/Expr.h:126
+if (TD)
+  D = D | DependencyFlags::Type;
+if (VD)

Mordante wrote:
> Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D |= 
> DependencyFlags::Type;` ? The latter seems to be more common.
Would also prefer `D |=`, but it leads to compilation errors.

The builtin `operator |=` accepts ints and, therefore, fails on strongly-typed 
enums.
And, AFAIK, there's no way to redefine `operator |=` for non-class types.



Comment at: clang/include/clang/AST/Stmt.h:323
   };
   enum { NumExprBits = NumStmtBits + 9 };
 

Mordante wrote:
> Please use `enum { NumExprBits = NumStmtBits + 5 +  DependencyFlagsBits };` 
> to avoid bugs when the size changes.
Many thanks! Using it here was the reason I wanted to add the constant in the 
first place. Ended up forgetting about it.
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 235837.
ilya-biryukov added a comment.

- Use DependencyFlagsBits for computing NumExprBits
- Reformat


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920

Files:
  clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -511,10 +511,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  DependencyFlags Deps = DependencyFlags::None;
+  if (TypeDependent)
+Deps = Deps | DependencyFlags::Type;
+  if (ValueDependent)
+Deps = Deps | DependencyFlags::Value;
+  if (InstantiationDependent)
+Deps = Deps | DependencyFlags::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps = Deps | DependencyFlags::UnexpandedPack;
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12400,9 +12400,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependencies(DependencyFlags::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -111,84 +111,65 @@
   return TemplateArgument(Args.copy(Context));
 }
 
-bool TemplateArgument::isDependent() const {
+DependencyFlags TemplateArgument::getDependencies() const {
+  DependencyFlags Deps = DependencyFlags::None;
   switch (getKind()) {
   case Null:
 llvm_unreachable("Should not have a NULL template argument");
 
   case Type:
-return getAsType()->isDependentType() ||
-   isa(getAsType());
+Deps = getAsType()->getDependencies();
+if (isa(getAsType()))
+  Deps = Deps | DependencyFlags::Type;
+return Deps;
 
   case Template:
-return getAsTemplate().isDependent();
+// FIXME: add TemplateName::getDependencies.
+if (getAsTemplate().isDependent())
+  Deps = Deps | DependencyFlags::TypeValue;
+if (getAsTemplate().isInstantiationDependent())
+  Deps = Deps | DependencyFlags::Instantiation;
+if (getAsTemplate().containsUnexpandedParameterPack())
+  Deps = Deps | DependencyFlags::UnexpandedPack;
+return Deps;
 
   case TemplateExpansion:
-return true;
+return DependencyFlags::TypeValueInstantiation;
 
-  case Declaration:
-if (DeclContext *DC = dyn_cast(getAsDecl()))
-  return DC->isDependentContext();
-return getAsDecl()->getDeclContext()->isDependentContext();
+  case Declaration: {
+auto *DC = dyn_cast(getAsDecl());
+if (!DC)
+  DC = getAsDecl()->getDeclContext();
+if (DC->isDependentContext())
+  Deps = DependencyFlags::TypeValueInstantiation;
+return Deps;
+  }
 
   case NullPtr:
-return false;
-
   case Integral:
-// Never dependent
-return false;
+return DependencyFlags::None;
 
   case Expression:
-return (getAsExpr()->isTypeDependent() || getAsExpr()->isValueDependent() ||
-isa(getAsExpr()));
+Deps = getAsExpr()->getDependencies();
+if (isa(getAsExpr()))
+  Deps = Deps | DependencyFlags::TypeValueInstantiation;
+return Deps;
 
   case Pack:
 for (const auto  : pack_elements())
-  if (P.isDependent())
-return true;
-return false;
+  Deps = Deps | P.getDependencies();
+return 

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

I like the goal of this patch and the simplifications it does. I don't feel 
qualified to do a full review.




Comment at: clang/include/clang/AST/Expr.h:126
+if (TD)
+  D = D | DependencyFlags::Type;
+if (VD)

Just curious why do you prefer `D = D | DependencyFlags::Type;` over `D |= 
DependencyFlags::Type;` ? The latter seems to be more common.



Comment at: clang/include/clang/AST/Stmt.h:323
   };
   enum { NumExprBits = NumStmtBits + 9 };
 

Please use `enum { NumExprBits = NumStmtBits + 5 +  DependencyFlagsBits };` to 
avoid bugs when the size changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61128 tests passed, 0 failed 
and 728 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:17
 
+using namespace clang::ast_matchers;
+

NOTE:
We need this to fix compiler errors down below. There's an AST matcher called 
`isTypeDependent` and we add a function 
`clang::isTypeDependent(DependencyFlags)` in this change.

Overload resolution does its job, but we have to make sure they're in the same 
namespace, so we need to put `using namespace` somewhere inside the `clang` 
namespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Also note that this change does not move any code around to make sure this 
change is easy to review and validate that the code is doing the same thing.
I'm also planning to move all the code that computes dependencies into one 
place (it's currently scattered around many files and it certainly makes it 
quite hard to find bugs in the code and update it). But I would really like to 
get everyone aligned on the direction we're taking here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.
Herald added a subscriber: rnkovacs.

@rsmith, could you please take a look and let me know whether you think adding 
a new type for this makes sense?

On one hand, I feel it's good to have a type to represent "dependencies" and it 
allow to write helper functions and easily add new bits into the dependencies 
without rewriting all the code propagating those flags (error bit).

OTOH, using this for some things might be confusing:

- a type cannot be value-dependent, but dependency flags allow for that.
- a template argument can currently be "dependent" after can be either type- or 
value-dependent, also confusing.
- ...

Note there are still some rough edges here, see the added FIXMEs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71920



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

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: rsmith.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.
Herald added a project: clang.

This changes introduces an enum to represent dependencies as a bitmask
and extract common patterns from code that computes dependency bits into
helper functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71920

Files:
  clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
  clang/include/clang/AST/DependencyFlags.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprObjC.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp

Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -511,10 +511,23 @@
 void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
-  E->setTypeDependent(Record.readInt());
-  E->setValueDependent(Record.readInt());
-  E->setInstantiationDependent(Record.readInt());
-  E->ExprBits.ContainsUnexpandedParameterPack = Record.readInt();
+
+  // FIXME: write and read all DependentFlags with a single call.
+  bool TypeDependent = Record.readInt();
+  bool ValueDependent = Record.readInt();
+  bool InstantiationDependent = Record.readInt();
+  bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  DependencyFlags Deps = DependencyFlags::None;
+  if (TypeDependent)
+Deps = Deps | DependencyFlags::Type;
+  if (ValueDependent)
+Deps = Deps | DependencyFlags::Value;
+  if (InstantiationDependent)
+Deps = Deps | DependencyFlags::Instantiation;
+  if (ContainsUnexpandedTemplateParameters)
+Deps = Deps | DependencyFlags::UnexpandedPack;
+  E->setDependencies(Deps);
+
   E->setValueKind(static_cast(Record.readInt()));
   E->setObjectKind(static_cast(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12420,9 +12420,7 @@
   // base classes.
   CallExpr *CE = CallExpr::Create(Context, Fn, Args, Context.DependentTy,
   VK_RValue, RParenLoc);
-  CE->setTypeDependent(true);
-  CE->setValueDependent(true);
-  CE->setInstantiationDependent(true);
+  CE->addDependencies(DependencyFlags::TypeValueInstantiation);
   *Result = CE;
   return true;
 }
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -111,84 +111,65 @@
   return TemplateArgument(Args.copy(Context));
 }
 
-bool TemplateArgument::isDependent() const {
+DependencyFlags TemplateArgument::getDependencies() const {
+  DependencyFlags Deps = DependencyFlags::None;
   switch (getKind()) {
   case Null:
 llvm_unreachable("Should not have a NULL template argument");
 
   case Type:
-return getAsType()->isDependentType() ||
-   isa(getAsType());
+Deps = getAsType()->getDependencies();
+if (isa(getAsType()))
+  Deps = Deps | DependencyFlags::Type;
+return Deps;
 
   case Template:
-return getAsTemplate().isDependent();
+// FIXME: add TemplateName::getDependencies.
+if (getAsTemplate().isDependent())
+  Deps = Deps | DependencyFlags::TypeValue;
+if (getAsTemplate().isInstantiationDependent())
+  Deps = Deps | DependencyFlags::Instantiation;
+if (getAsTemplate().containsUnexpandedParameterPack())
+  Deps = Deps | DependencyFlags::UnexpandedPack;
+return Deps;
 
   case TemplateExpansion:
-return true;
+return DependencyFlags::TypeValueInstantiation;
 
-  case Declaration:
-if (DeclContext *DC = dyn_cast(getAsDecl()))
-  return DC->isDependentContext();
-return getAsDecl()->getDeclContext()->isDependentContext();
+  case Declaration: {
+auto *DC = dyn_cast(getAsDecl());
+if (!DC)
+  DC = getAsDecl()->getDeclContext();
+if (DC->isDependentContext())
+  Deps = DependencyFlags::TypeValueInstantiation;
+return Deps;
+  }
 
   case NullPtr:
-return false;
-
   case Integral:
-// Never dependent
-return false;
+return DependencyFlags::None;
 
   case Expression:
-return (getAsExpr()->isTypeDependent() || getAsExpr()->isValueDependent() ||
-isa(getAsExpr()));
+Deps = getAsExpr()->getDependencies();
+if (isa(getAsExpr()))
+  Deps = Deps | DependencyFlags::TypeValueInstantiation;
+return Deps;
 
   case Pack:
 for (const auto  : pack_elements())
-