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

[Eric Fiselier via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D37035#849574, @rsmith wrote:

> I don't like the model of conditionally rebuilding the default initializer / default argument if it contains one of these builtins; it seems more heavy-handed than necessary. I'm also not convinced that we need to store the computed value in the AST.
>
> As an alternative, `CodeGen` and the expression evaluator can track a current call context (the innermost call expression or list initialization that is not within a default argument / default member initializer) consisting of the enclosing `FunctionDecl` and `SourceLocation`, and when the value of one of these builtins is needed, pass that context to the AST node to ask what value it has in that context. This will require a bit more work for the static analyzer, but should be fairly simple elsewhere and will avoid the extra AST nodes for the desugared form.


That sounds good to me. Could you suggest where said context should correctly live?



================
Comment at: docs/LanguageExtensions.rst:2118
+point is the location of the caller. When the builtins appear as part of a
+NSDMI the invocation point is the location of the constructor or
+aggregate initialization used to create the object. Otherwise the invocation
----------------
rsmith wrote:
> What is an NSDMI? I think you mean "default member initializer".
"Non-static default member initializer". I'll change it to your suggestion.


================
Comment at: include/clang/AST/Expr.h:3838
+  /// function.
+  const char *getBuiltinStr() const LLVM_READONLY;
+
----------------
majnemer wrote:
> StringRef?
Sure. Why not. I'm never sure how to handle string literals.


https://reviews.llvm.org/D37035