This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][PlistMacroExpansion] New flag to convert macro expansions to events
AbandonedPublic

Authored by Szelethus on Oct 2 2018, 10:29 AM.

Diff Detail

Event Timeline

Szelethus created this revision.Oct 2 2018, 10:29 AM
Szelethus updated this revision to Diff 168315.Oct 4 2018, 9:43 AM
Szelethus updated this revision to Diff 168317.Oct 4 2018, 9:46 AM

Changed the plist output.

Szelethus updated this revision to Diff 168319.Oct 4 2018, 9:47 AM
whisperity accepted this revision.Oct 16 2018, 11:11 AM

Remove the custom file paths and URLs but other than that this is pretty trivial.

This revision is now accepted and ready to land.Oct 16 2018, 11:11 AM
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
6

Could we figure out how not to have hardcoded contributor-based strings there? Maybe have a python script generating those / doing a cleanup?

Szelethus added inline comments.Oct 16 2018, 11:37 AM
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
6

I'm just about to remove it, as I did for all the other [PlistMacroExpancion] patches.

But yea, a tool wouldn't hurt.

Removed the version from the plist files, rebased to part 5.

NoQ accepted this revision.Nov 1 2018, 6:53 PM

Looks great, let's land? Not sure if i already asked - am i understanding correctly that this is a "poor-man's" support for macro expansions for tools that read plists but do not support those new plist macro dictionaries yet? Also i wanted to have -analyzer-output=text tests in order to easily see how exactly macro expansions turn into events, but i guess it doesn't make sense because this feature is only for -analyzer-output=plist because other outputs have their own mechanisms for handling macros so nvm^^

I wonder how expansions of huge macros will look as events.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
289–290

I think the proper way of using Twine is to only wrap the first value with it. Otherwise string concatenation is done normally and then a single string is converted into a Twine, which doesn't give any benefits.

I also would like to see in a tool how this would look like as an event before committing :) Just a sanity check to make sure this feature makes sense. Could you post a screenshot of CodeChecker or any other tool using this feature?

In D52790#1285039, @NoQ wrote:

Looks great, let's land?

I'll probably land it after part 5, in order to ease on rebasing.

Not sure if i already asked - am i understanding correctly that this is a "poor-man's" support for macro expansions for tools that read plists but do not support those new plist macro dictionaries yet?

Correct, same as notes-as-events.

I wonder how expansions of huge macros will look as events.

Quite funny, actually :D Think about assert, it's quite a mouthful. I'll post screenshots when I have more time.

Szelethus planned changes to this revision.Dec 5 2018, 12:05 PM

Since the way how these macro events are created as a whole changed in part 1, this path will need a big overhaul.

Szelethus abandoned this revision.Jan 26 2019, 1:44 PM

As we currently support the actual macro_expansions format in CodeChecker, and AFAIK you guys at Apple prefer relying on Xcode (http://lists.llvm.org/pipermail/cfe-dev/2018-September/059231.html), I'd like to focus on more pressing matters.

I personally do use HTML output quite a lot (and we do have non-Xcode projects), and complex macros in HTML are currently totally unusable.
I'm not sure whether this is a right approach to handling this, but it's definitely a problem for us.
In general - thanks for working on this anyway.