[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

[Balázs Benics via Phabricator via cfe-commits]

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

I want to highlight, that the 4th part of the stack is not yet done.
Partially because I'm not quite familiar with CTU.
AFAIK, CTU creates compiler invocations, which are going to call Parse at some point.
I'm not sure how to configure, apply the callbacks, etc. on the Preprocessor in the middle of that process.
I'm also reluctant to pass this domain-specific `MacroExpansionContext` as the 23rd parameter to that generic function (`ASTUnit::LoadFromCommandLine`).

I'm not even sure if there will be a Preprocessor, which would generate the 'OnToken' or PPCallbacks calls if we are parsing a PCH or some other serialized source.
That would be crucial for this MacroExpansion machinery. I hope you can ensure that I'm on the right path.



================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:79-80
+  ///         substituted in place of the macro.
+  ///         If no macro was expanded at that location, returns an empty
+  ///         string.
+  MacroExpansionText
----------------
martong wrote:
> I think we should have an Optional<String> as a return type, so we'd be able to detect if no macro expansion happened. (?)
Yes, it depends on what interface we want to provide.
If we expect it to be called on source locations that participate in a macro expansion, then it would be somewhat redundant.
I don't mind if it returns an Optional<>.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:81
+  ///         string.
+  MacroExpansionText
+  getExpandedMacroForLocation(SourceLocation MacroExpansionLoc) const;
----------------
martong wrote:
> Could this be `StringRef` also?
We could return a `StringRef`, but that would point to an entry of a DenseMap. That could be invalidated later if the preprocessor still generates tokens.

If we say that these calls should be called after the preprocessing is completely done, it could just return a `StringRef`.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:82
+  MacroExpansionText
+  getExpandedMacroForLocation(SourceLocation MacroExpansionLoc) const;
+
----------------
martong wrote:
> `getExpandedText` ? Since what you get back is not a macro anymore.
You are right! I like it.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:87-88
+  ///         the macro expansion chain from the given location.
+  ///         If no macro was expanded at that location, returns an empty
+  ///         string.
+  StringRef
----------------
martong wrote:
> We should be able to distinguish between no macro expansion (1) and expansion to an empty string (2). So, I think we should have an Optional<StringRef> as a return type.
I'm not sure about this.
If a SourceLocation is not a macro expansion, then the `isFileID()` would return false AFAIK.
Otherwise, if the result is an empty string, that means that the macro indeed expanded to no tokens.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:90
+  StringRef
+  getSubstitutedTextForLocation(SourceLocation MacroExpansionLoc) const;
+
----------------
martong wrote:
> `Substituted` is a bit ambiguous to me. What about `getOriginalText` ?
I agree.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:22
+    : PP(PP), SM(PP.getSourceManager()), LangOpts(LangOpts) {
+  class MacroExpansionRangeRecorder : public PPCallbacks {
+    const Preprocessor &PP;
----------------
xazax.hun wrote:
> It may be more idiomatic to put classes in an anonymous namespace rather than expanding them in a method.
I tried, but I could not hide the `MacroExpansionRangeRecorder` class as a detail.
https://godbolt.org/z/bcYK7x
Let me know if there is a better way to hide details.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:39
+
+      SourceLocation ExpansionEnd = [Range, &SM = SM, &MacroName] {
+        // If the range is empty, use the length of the macro.
----------------
xazax.hun wrote:
> martong wrote:
> > ?
> Why do you need the assignment in `&SM = SM`?
`SM` is an object member.
The lambda should either capture the `this` pointer or a pointer/reference to the referred member (`SM`).
I preferred the latter as the lambda doesn't want to access all of the members except this one.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:94
+  if (it == ExpandedTokens.end())
+    return MacroExpansionText("");
+  return it->getSecond();
----------------
xazax.hun wrote:
> I am a bit uneasy about this. As far as I understand a macro could be expanded to an empty string. How could the user differentiate between a valid empty expansion and a lookup failure. Also, can this lookup ever fail in a well behaving code? Do we need to add `llvm_unreachable()` or something?
Yes, I should have put an `unreachable` there as you suggested.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:105
+  if (it == ExpansionRanges.end())
+    return "";
+
----------------
xazax.hun wrote:
> Similar concerns to above. Do we want an assert? When can this happen?
Yes, I should put an assert there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93222