[PATCH] D23829: [ELF] - Use std::regex instead of hand written logic in elf::globMatch()

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:95
@@ -94,3 +94,3 @@
 bool LinkerScript<ELFT>::shouldKeep(InputSectionBase<ELFT> *S) {
-  for (StringRef Pat : Opt.KeptSections)
-    if (globMatch(Pat, S->getSectionName()))
+  for (Regex *Pat : Opt.KeptSections) {
+    StringRef Name = S->getSectionName();
----------------
ruiu wrote:
> Nit: we probably should use `Re` as a local variable name throughout this patch instead of `Pat`, `Pattern` nor `Patterns` because it is no longer a glob pattern (which `Pat` was named after) but a regular expression.
Done.

================
Comment at: ELF/LinkerScript.cpp:847-852
@@ -842,8 +846,8 @@
 
-std::vector<StringRef> ScriptParser::readInputFilePatterns() {
-  std::vector<StringRef> V;
+std::vector<Regex> ScriptParser::readInputFilePatterns() {
+  std::vector<Regex> V;
   while (!Error && !skip(")"))
-    V.push_back(next());
+    V.push_back(toRegex(next()));
   return V;
 }
 
----------------
ruiu wrote:
> It shouldn't be slow. I did not actually take a look at the implementation, but at least in theory, any regular expression that we use can be compiled to a DFA because we do not use anything that requires NFA such as capturing. If you can't use `|` because you are using the basic re, you shouldn't use the basic re but instead extended one.
Ok, I see.
Found one more problem with that unfortunatelly:

I can't just switch current:
  std::vector<llvm::Regex> ExcludedFiles;
  std::vector<llvm::Regex> SectionPatterns;

to 

llvm::Regex ExcludedFiles;
llvm::Regex SectionPatterns;

Because as I mantioned above, llvm::Regex does not have constructor with no arguments. 
Not sure we what we should do (I guess anyways we will switch to use of std::regex one day probably), so possible solutions are
either not to use "|" or to make them unique_ptr.
What do you think ?

================
Comment at: ELF/LinkerScript.h:97
@@ -95,3 +96,3 @@
   static bool classof(const BaseCommand *C);
-  StringRef FilePattern;
+  std::unique_ptr<llvm::Regex> FilePattern;
   SortKind SortOuter = SortNone;
----------------
ruiu wrote:
> Why unique_ptr?
In compare with std::regex, llvm::Regex does not have constructor with no arguments. 
Removed unique_ptr, added argument for InputSectionDescription instead.

================
Comment at: ELF/LinkerScript.h:132
@@ -130,3 +131,3 @@
   // be kept even if they are unused and --gc-sections is specified.
-  std::vector<StringRef> KeptSections;
+  std::vector<llvm::Regex *> KeptSections;
 };
----------------
ruiu wrote:
> Why pointers?
Restriction of API. llvm::Regex is non-copyable.
That looks to be intended:
 r198334 "Make llvm::Regex non-copyable but movable."

So I cant just copy kept patterns into KeptSections.


================
Comment at: ELF/LinkerScript.h:106
@@ -103,1 +105,3 @@
+  std::vector<llvm::Regex> ExcludedFiles;
+  std::vector<llvm::Regex> SectionPatterns;
 };
----------------
Not sure, do we want to rename SectionPatterns and FilePattern to something then ?


https://reviews.llvm.org/D23829