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

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

steakhal marked 4 inline comments as done.
steakhal added a comment.

Thank you @Szelethus for taking the time to review this.
This time I marked the inline comments done where it was applicable :)

I'm gonna investigate some of your comments and if everything goes well I'm planning to commit this in the next week.



================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:64-66
+/// \remark Currently we don't respect the whitespaces between expanded tokens,
+///         so the output for this example might differ from the -E compiler
+///         invocation.
----------------
Szelethus wrote:
> That might be very tricky. I recall stumbling across the same issue when using the `Preprocessor`. I think I used this or something similar:
> 
> https://clang.llvm.org/doxygen/classclang_1_1Preprocessor.html#adb53a8b33c6ea7a5e0953126da5fb121
Since I'm listening to every new token that was parsed (via the token watcher), the tokens will be absolutely correct. But I'm sacrificing the precise location to acquire it.
However, we should be able to reconstruct the precise position too with some additional bookkeeping. But I'm more interested in correct expansions than whitespace preserving (and also correct) expansions.
That's why I'm not interested in re-lexing anything via `clang::Preprocessor::getRawToken`.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:91
+
+  /// \param MacroExpansionLoc Must be the expansion location of a macro.
+  /// \return The text from the original source code which were substituted by
----------------
Szelethus wrote:
> Consider mentioning `getExpansionLoc`, since that is almost always the source of the expansion loc (no other comes to my mind).
Ok, I will.


================
Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:105
+  using MacroExpansionText = SmallString<40>;
+  using ExpansionMap = llvm::DenseMap<SourceLocation, MacroExpansionText>;
+  using ExpansionRangeMap = llvm::DenseMap<SourceLocation, SourceLocation>;
----------------
Szelethus wrote:
> Hmm, I'm by no means an expert, but isn't a string-like structure a bit big for a `DenseMap`?
I'm not sure. When I grepped for an associative container having SourceLocation as a key, the only one I found was the DenseMap.
Besides that, the Value should fit into a single cacheline (at least on my X86_64 linux machine).
What data structure you think is a better fit?


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:41-43
+      if (Range.getBegin() == Range.getEnd())
+        return SM.getExpansionLoc(
+            MacroName.getLocation().getLocWithOffset(MacroName.getLength()));
----------------
Szelethus wrote:
> Well that is interesting, so the `Range` parameter is **not** (always?) the range of the expansion. [[ https://clang.llvm.org/doxygen/classclang_1_1PPCallbacks.html#a1f99f55fc3c5df84b152f9bb2057633f | Doxygen]] says that 
> 
> > Called by Preprocessor::HandleMacroExpandedIdentifier when a macro invocation is found.
> 
> in which there is a TODO:
> 
> ```
>           // FIXME: We lose macro args info with delayed callback.
>           Callbacks->MacroExpands(Info.Tok, Info.MD, Info.Range,
>                                   /*Args=*/nullptr);
> ```
> 
> I suppose you're handling this corner case?
It could be, but I'm not sure.
According to the code snippet you mention, the range does not look suspicious. The `nullptr` is passed as the `Args` parameter.
So, I think it's a different thing.

Either way, my tests will break if anything changes, so we will be notified for sure.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:183-186
+    // FIXME: For now, we don't respect whitespaces between macro expanded
+    // tokens. We just emit a space after every identifier to produce a valid
+    // code for `int a ;` like expansions.
+    //              ^-^-- Space after the 'int' and 'a' identifiers.
----------------
Szelethus wrote:
> In the  `TokenPrinter` in the plist implementation, I used `Preprocessor::getSpelling()`.
Oh, I'm gonna investigate!
This code looks really ugly xD.


================
Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:210
+}
\ No newline at end of file

----------------
steakhal wrote:
> martong wrote:
> > Missing newline?
> I honestly don't know why was that not addressed by clang-format.
> Even here, the prebuild bot should have complained about this - if this is an issue.
> 
> If you still think I should add that newline, we should also take a look at the `.clang-format`.
Fixed.


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

https://reviews.llvm.org/D93222