Szelethus accepted this revision. Szelethus added a comment. Herald added a subscriber: nullptr.cpp.
This is amazing. We longed for a sensible implementation for this for a long time. Really liking the unit tests as well! There are a number of inlines you didn't mark as done but seem to have addressed. > The main goal is to substitute the current macro expansion generator in the > PlistsDiagnostics, but all the other DiagnosticsConsumer could benefit from > this. Good! Burn it, bury it, lets forget that it ever existed. It was a stupid idea from the get-go and got so much worse over time. ================ 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. ---------------- 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 ================ 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 ---------------- Consider mentioning `getExpansionLoc`, since that is almost always the source of the expansion loc (no other comes to my mind). ================ Comment at: clang/lib/Analysis/MacroExpansionContext.cpp:41-43 + if (Range.getBegin() == Range.getEnd()) + return SM.getExpansionLoc( + MacroName.getLocation().getLocWithOffset(MacroName.getLength())); ---------------- 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? ================ 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. ---------------- In the `TokenPrinter` in the plist implementation, I used `Preprocessor::getSpelling()`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93222/new/ https://reviews.llvm.org/D93222 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
