Adds a MacroExpansionContext member to the AnalysisConsumer class.
Tracks macro expansions only if the ShouldDisplayMacroExpansions is set.
Passes a reference down the pipeline letting AnalysisConsumers query macro
expansions during bugreport construction.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks quite straight-forward other than that new prototype.
clang/include/clang/Analysis/PathDiagnostic.h | ||
---|---|---|
911 ↗ | (On Diff #311596) | Why do we need this prototype here? |
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp | ||
---|---|---|
129 | This will always record macro related information right? Some of the diagnostic consumers might not ever touch macro expansions. I think it would be great to only record expansions when we actually will end up using them. Alternatively, a benchmark proving the runtime of recording negligible on macro heavy code (boost maybe?) might be sufficient. |
Thanks for the review.
clang/include/clang/Analysis/PathDiagnostic.h | ||
---|---|---|
911 ↗ | (On Diff #311596) | Thanks. I probably accidentally let it there :D I will remove this. |
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp | ||
129 | To be fair, we already have an analyzer config exactly for this. |
Updates:
- New the construction of MacroExpansionContext won't hook the Preprocessor in the constructor. Hooking is done via the registerForPreprocessor(PP) member function.
- Hooking the PP now depends on the ShouldDisplayMacroExpansions analyzer option.
- Both getExpandedText and getOriginalText returns Optional<StringRef>. Semantics and comments changed accordingly. If the ShouldDisplayMacroExpansions analyzer flag is not set, both of these functions always return None.
- Removed the accidental createHTMLSingleFileDiagnosticConsumer prototype in PathDiagnostic.h.
- New unittest case added NoneForNonExpansionLocations.
- The rest of the tests were uplifted to unwrap the Optional to accommodate the new API.
- The last assertion of the RedefUndef unittest case was changed to match the new behavior for non-expansion locations.
Nothing changed besides moving some parts to the parent revision.
- registerForPreprocessor member function
- Optional<StringRef> return type
- NoneForNonExpansionLocations unittest-case
Still looks good.
(The parent is an NFC and only this patch changes the behavior (and only if the cmdline flag is there), right? So, I'd expect the big impact from this patch.)
My no1. thought is that I wish the new functionality you're implementing was in the interface of the Preprocessor. I found, and still find it so hard to believe that you couldn't just retrieve this information -- your projects seems to plug this gaping hole in the design. I respect that this is an opt-in functionality for now though, and I guess we could make this a default feature with relative ease.
The actual patch is straightforward, LGTM!
It is hard to believe, but it is the case.
your projects seems to plug this gaping hole in the design.
I honestly think that it is the supposed way of doing this. The chained preprocessor callbacks and the token watcher fit perfectly into this picture.
I respect that this is an opt-in functionality for now though, and I guess we could make this a default feature with relative ease.
We should carefully evaluate the impact and stability of this before making it happen.
This will always record macro related information right? Some of the diagnostic consumers might not ever touch macro expansions. I think it would be great to only record expansions when we actually will end up using them. Alternatively, a benchmark proving the runtime of recording negligible on macro heavy code (boost maybe?) might be sufficient.