[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:2179
+Clang provides experimental builtins to support C++ standard library implementation
+of `std::experimental::source_location` as specified in  http://wg21.link/N4600.
+With the exception of `__builtin_COLUMN`, these builtins are also implemented by
----------------
rsmith wrote:
> rST uses double-backticks for code font. Does this URL get autolinked?
Woops. Fixed.


================
Comment at: lib/AST/APValue.cpp:46-49
+template <class T> static bool isEmptyOrTombstoneKey(T const &V) {
+  using DMI = llvm::DenseMapInfo<T>;
+  return V == DMI::getEmptyKey() || V == DMI::getTombstoneKey();
+}
----------------
rsmith wrote:
> This seems to be unused?
It was used, but I folded the definition into that usage.


================
Comment at: lib/AST/ExprConstant.cpp:3353
+      APValue Str(LVal.Base, CharUnits::Zero(), APValue::NoLValuePath(), 0);
+      CompleteObject StrObj(&Str, LVal.Base.getGlobalStringType()->desugar(),
+                            false);
----------------
rsmith wrote:
> Similarly, why do you need to desugar the type here?
It does seem redundant. Removing.

I think I was using it to generate a QualType from a Type*.


================
Comment at: lib/CodeGen/CGExprConstant.cpp:867-869
-  llvm::Constant *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *DAE, QualType T) {
-    return Visit(DAE->getExpr(), T);
-  }
----------------
rsmith wrote:
> Why was this removed?
I believe it was dead code?


================
Comment at: lib/CodeGen/CGExprConstant.cpp:868-869
   llvm::Constant *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE, QualType T) {
+    // FIXME: Is it ever possible for a SourceLocExpr to be below this node?
+    // If so, we need to update the CurSourceLocExprScope in CodeGenFunction.
+
----------------
rsmith wrote:
> I think the salient point here is: this visitor will fail if it reaches a `SourceLocExpr`, so there's no need to track state that only it needs. (Same reason we didn't need a `CXXDefaultInitExprScope` for handling `CXXThisExpr`.)
Ack.  Removing the comment.


https://reviews.llvm.org/D37035