[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: docs/LanguageExtensions.rst:2118
+point is the location of the caller. When the builtins appear as part of a
+default member initializer, the invocation point is the location of the
+constructor or aggregate initialization used to create the object. Otherwise
----------------
FIXME: This texts sucks. I can do better.


================
Comment at: include/clang/AST/Decl.h:2418
   FieldDecl(Kind DK, DeclContext *DC, SourceLocation StartLoc,
-            SourceLocation IdLoc, IdentifierInfo *Id,
-            QualType T, TypeSourceInfo *TInfo, Expr *BW, bool Mutable,
+            SourceLocation IdLoc, IdentifierInfo *Id, QualType T,
+            TypeSourceInfo *TInfo, Expr *BW, bool Mutable,
----------------
`git clang-format` strikes again...


================
Comment at: lib/AST/Expr.cpp:1895
+  switch (getIdentType()) {
+  default:
+    llvm_unreachable("should not be here");
----------------
Remove default case to make way for diagnostics when new cases are added.


================
Comment at: lib/AST/ExprConstant.cpp:4567
       return Error(E);
+    SourceLocDefaultMemberInitContextRAII Guard(Info, E->getLocStart(),
+                                                E->getUsedContext());
----------------
`E->getUsedLocation()`?


================
Comment at: lib/AST/ExprConstant.cpp:7134
+      Loc = Info.EvaluatingDefaultMemberInit->Loc;
+    else if (Info.EvaluatingDefaultArg && Info.EvaluatingDefaultArg->isInSameCurrentCall())
+      Loc = Info.EvaluatingDefaultArg->Loc;
----------------
This special control flow should be tested. I think it might be? But not against a NSDMI.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1250
 public:
+public:
+  class SourceLocExprScopeBase;
----------------
redundant `public`.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1253
+
+  SourceLocExprScopeBase *CurCXXDefaultArgScope = nullptr;
+  SourceLocExprScopeBase *CurCXXDefaultInitScope = nullptr;
----------------
These need documentation.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1259
+
+    static bool ShouldEnable(CodeGenFunction &CGF) {
+      return CGF.CurCXXDefaultArgScope == nullptr ||
----------------
Needs documentation.


================
Comment at: lib/CodeGen/CodeGenFunction.h:1339
 
+  class CXXDefaultArgExprScope : public SourceLocExprScopeBase {
+    using Base = SourceLocExprScopeBase;
----------------
Needs documentation.


https://reviews.llvm.org/D37035