baloghadamsoftware requested changes to this revision.
baloghadamsoftware added inline comments.
This revision now requires changes to proceed.
================
Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:49
+ string PackageName = name;
+ list<CmdLineOption> PackageOptions;
+ Package ParentPackage;
----------------
Please add a comment that this field is optional.
================
Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:87
+ string HelpText;
+ list<CmdLineOption> CheckerOptions;
+ list<Checker> Dependencies;
----------------
Same as above.
================
Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:156
+ return FullName == Rhs.FullName;
+ }
+
----------------
Are we able now to distinguish checkers of the same short names but in
different packages? In earlier versions the short name had to be different to
avoid conflicts.
================
Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:282
+ /// \c Checkers field. For convenience purposes.
+ llvm::StringMap<size_t> PackageSizes;
----------------
Cannot we store the size of the `Package` in the `Package` itself?
================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:50
+ }
+};
+
----------------
Could we maybe use `std::less<CheckerOrPackageInfo>`?
================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:68
+ CheckerOrPackageFullNameLT{}) &&
+ "Can't do binary search on a non-sorted collection!");
+
----------------
I would better keep the original message here. It has a more formal style.
================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:181
+ llvm::sort(Checkers, CheckerNameLT{});
+ llvm::sort(Packages, PackageNameLT{});
----------------
We always have packages first. Please swap these lines.
================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:307
+ assert(CheckerIt != Checkers.end() &&
+ "Failed to find the checker while attempting to set up it's "
+ "dependencies!");
----------------
Typo: it's -> its.
================
Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:371
+ // We can't get away with binaryFind here, as it uses lower_bound.
+ auto CheckerIt = llvm::find(Checkers, CheckerInfo(SuppliedChecker));
+ if (CheckerIt != Checkers.end())
----------------
Please explain the problem with `std::lower_bound()`. I do not see why we
cannot use it (followed by a check whether we found an element equal to the
searched element, not a greater one). I cannot see any excuse to use linear
search instead of binary one in a sorted collection.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57855/new/
https://reviews.llvm.org/D57855
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
