[PATCH] D39212: Check special-case-list regex before insertion.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Support/SpecialCaseList.cpp:40
   }
   Trigrams.insert(Regexp);
 
----------------
vlad.tsyrklevich wrote:
> vsk wrote:
> > CC'ing @vlad.tsyrklevich 
> > 
> > You might prevent more bugs by having TrigramIndex::insert check for empty strings and return an Error. Moving the check there should also make it easier to add a unit test (see TrigramIndexTest.cpp).
> > 
> In this case the exception is actually hit before we even get to the TrigramIndex--it's on insertion into Strings since the empty string is a literal. I'm not sure if TrigramIndex should try to check regexp validity. That seems like a complicated thing to do and TrigramIndex would only be able to look for the empty string. If this were actually to hit the regexp case we would have caught it with the isValid() check below.
Sorry, I misread the backtrace and assumed the error occurred when inserting into Trigrams. I agree with what you've said.

It would still be useful to add a test along with this patch (SpecialCaseListTest.cpp):
```
@@ -67,6 +67,9 @@ TEST_F(SpecialCaseListTest, SectionRegexErrorHandling) {
 
   EXPECT_EQ(makeSpecialCaseList("[[]", Error), nullptr);
   EXPECT_TRUE(((StringRef)Error).startswith("malformed regex for section [: "));
+
+  EXPECT_EQ(makeSpecialCaseList("src:=", Error), nullptr);
+  EXPECT_TRUE(((StringRef)Error).endswith("Supplied regexp was blank"));
 }
```


https://reviews.llvm.org/D39212