This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Fix inconsistencies in AnalyzerOptions
ClosedPublic

Authored by Szelethus on Oct 15 2018, 2:36 AM.

Details

Summary

I was a little unsure about the title, but it is what it is.

I'm in the process of refactoring AnalyzerOptions. The main motivation behind here is to emit warnings if an invalid -analyzer-config option is given from the command line, and be able to list them all.

This first NFC patch contains small modifications to make AnalyzerOptions.cpp a little more consistent.

Diff Detail

Event Timeline

Szelethus created this revision.Oct 15 2018, 2:36 AM
Szelethus retitled this revision from [analyzer][NFC] Tighten up AnalyzerOptions to [analyzer][NFC] Fix inconsistencies in AnalyzerOptions.Oct 15 2018, 4:16 AM
Szelethus updated this revision to Diff 169703.Oct 15 2018, 7:31 AM

The main motivation behind here is to emit warnings if an invalid

I'm totally with you here, but IIRC (@NoQ might want to correct me here),
the design decision was made specifically to ignore incorrect options, so that e.g. old versions of Xcode used with old projects would still work.

I'm not sure why you could get away with removing those llvm_unreachable cases?

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
373

Why?

418

Why?

lib/StaticAnalyzer/Core/CoreEngine.cpp
72

Why?

I'm not sure why you could get away with removing those llvm_unreachable cases?

Because I got a warning for using default when every enum value was handled in the switch. Since whether the flag is set or not can be retrieved via llvm::Optional::hasValue(), I removed the .*NotSet flags. The future maintainer who adds a new value will get a warning for not handling it anyways.

The main motivation behind here is to emit warnings if an invalid

I'm totally with you here, but IIRC (@NoQ might want to correct me here),
the design decision was made specifically to ignore incorrect options, so that e.g. old versions of Xcode used with old projects would still work.

I intend to emit a warning, but go on with the analysis. That's fair I believe?

OK that makes sense to me, just let's be careful not to crash in places where we weren't crashing before.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
353

I'm slightly confused, where is this function called?
Same for the function above?

Szelethus marked 2 inline comments as done.Oct 15 2018, 3:32 PM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
353

Its used in D53277, but this addition belongs here more.

Granted, I should've refactored this file to demonstrate why I made this change, but in D53277 I literally delete everything, so I didn't bother.

NoQ accepted this revision.Oct 19 2018, 6:24 PM

Thanks! Let's commit.

This revision is now accepted and ready to land.Oct 19 2018, 6:24 PM
This revision was automatically updated to reflect the committed changes.
Szelethus marked 3 inline comments as done.