[PATCH] D52794: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 10 inline comments as done.
Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:667
+
+//===----------------------------------------------------------------------===//
+// Declarations of helper functions and data structures for expanding macros.
----------------
xazax.hun wrote:
> Maybe it would be worth to move these helpers to a separate file and add some documentation why we actually simulate the preprocessor rather than doing something else.
I'm actually a little unsure about that -- I have had similar thought, but the placement here can be justified with a simple "Where else?". But that's a little vague, I'll admit.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:724
+  TokenPrinter Printer(OS, PP);
+  return { getExpandedMacroImpl(Printer, MacroLoc, PP), OS.str() };
+}
----------------
xazax.hun wrote:
> Maybe could directly return ExpansionInfo?
I renamed the function to be less confusing. Did this address your concern?


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:753
+    // macro.
+    if (const MacroInfo *MI = PP.getMacroInfo(II)) {
+      getExpandedMacroImpl(Printer, T.getLocation(), PP);
----------------
Szelethus wrote:
> xazax.hun wrote:
> > This might not be completely correct. We are using the preprocessor state after processing the whole translation unit rather than the preprocessor state at the expansion location. This might not cause any problems unless there is a collision between macro names and non-macro names. Also, undefs, redefs might cause troubles.
> Yup, you're right, I'm afraid :/
> 
> I'll investigate whether I can gather the macro definition at the spelling location with `Lexer`.
I'm really-really happy it didn't have to come to that.


https://reviews.llvm.org/D52794