[PATCH] Introduce factory methods for SpecialCaseList

Alexey Samsonov samsonov at google.com
Fri Aug 9 01:29:04 PDT 2013



================
Comment at: include/llvm/Transforms/Utils/SpecialCaseList.h:106
@@ +105,3 @@
+  SpecialCaseList();
+  static bool parse(const MemoryBuffer *MB, SpecialCaseList &SCL,
+                    std::string &Error);
----------------
Peter Collingbourne wrote:
> Nitpick: why make this a static member function?
Fixed

================
Comment at: lib/Transforms/Utils/SpecialCaseList.cpp:103
@@ -72,2 +102,3 @@
   StringMap<StringMap<std::string> > Regexps;
+  SCL.Entries.clear();
   for (SmallVectorImpl<StringRef>::iterator I = Lines.begin(), E = Lines.end();
----------------
Peter Collingbourne wrote:
> This may leak memory if this function is called with a nonempty Entries, and it will do nothing if Entries is empty.  Since this function will only do something sensible if Entries is empty, you could assert it here instead.
Done

================
Comment at: lib/Transforms/Utils/SpecialCaseList.cpp:77
@@ +76,3 @@
+  if (Path.empty()) {
+    Error = "Empty path to blacklist file";
+    return 0;
----------------
Peter Collingbourne wrote:
> Why not s/blacklist/special case list/ here? (Actually I can think of a few reasons but I want to know yours.)
Good point. On a second thought, it's not the right place to print "blacklist" or "special case list" - we should let caller report the error message in the way he likes.

================
Comment at: lib/Transforms/Utils/SpecialCaseList.cpp:82
@@ +81,3 @@
+  if (error_code EC = MemoryBuffer::getFile(Path, File)) {
+    Error = (Twine("Can't open blacklist file: ") + Path + ": " +
+        EC.message()).str();
----------------
Peter Collingbourne wrote:
> Same here.
Same here.

================
Comment at: lib/Transforms/Utils/SpecialCaseList.cpp:76
@@ +75,3 @@
+    const StringRef Path, std::string &Error) {
+  if (Path.empty()) {
+    Error = "Empty path to blacklist file";
----------------
Peter Collingbourne wrote:
> This is a change in behaviour from the StringRef-taking constructor.  There should probably be a non-deprecated way to create an empty special case list, as each of the sanitizers currently needs to do when their special case list parameter is an empty string (which suggests that there should be a function somewhere (which perhaps ought to be this function) which preserves the original behaviour of the StringRef-taking constructor).
Changed this back to the original behavior.


http://llvm-reviews.chandlerc.com/D1327



More information about the llvm-commits mailing list