NoQ accepted this revision. NoQ added inline comments.
================ Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:146-149 +/// If you'd like to add a new -cc1 flag, add it to +/// include/clang/Driver/CC1Options.td, add a new field to store the value of +/// that flag in this class, and initialize it in +/// lib/Frontend/CompilerInvocation.cpp. ---------------- Szelethus wrote: > NoQ wrote: > > Nono, don't add more -cc1 flags :) > Code review is there to stop adding unnecessary -cc1 flags. Are we sure we > wouldn't even like to document it? I myself will add at least 2 more -cc1 > flags in the future (-analyzer-config-help, -analyzer-checker-option-help), > that undoubtedly belong there. But these flags wouldn't define new analyzer options(?) ================ Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:172 + .getAsInteger(10, *V); + assert(!HasFailed && "analyzer-config option should be numeric"); + (void)HasFailed; ---------------- Szelethus wrote: > NoQ wrote: > > I guess @xazax.hun's comment also applies to this assert. > Which is sadly only going to be addressed in followup patches :/ Note that I > merely moved code around in this patch, but have organized things in a way > that emitting proper diagnostics will be super easy going forward. Since > analyzer configs are mainly meant for developers, I think this is acceptable > before I fix it (in any case, it has always been like this). Hmm, right. I guess @xazax.hun's comment also applies to all other asserts all over this code. https://reviews.llvm.org/D53483 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
