<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 31, 2016 at 4:48 AM George Rimar via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added inline comments.<br>
<br>
================<br>
Comment at: ELF/LinkerScript.cpp:95<br>
@@ -94,3 +94,3 @@<br>
 bool LinkerScript<ELFT>::shouldKeep(InputSectionBase<ELFT> *S) {<br>
-  for (StringRef Pat : Opt.KeptSections)<br>
-    if (globMatch(Pat, S->getSectionName()))<br>
+  for (Regex *Pat : Opt.KeptSections) {<br>
+    StringRef Name = S->getSectionName();<br>
----------------<br>
ruiu wrote:<br>
> 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.<br>
Done.<br>
<br>
================<br>
Comment at: ELF/LinkerScript.cpp:847-852<br>
@@ -842,8 +846,8 @@<br>
<br>
-std::vector<StringRef> ScriptParser::readInputFilePatterns() {<br>
-  std::vector<StringRef> V;<br>
+std::vector<Regex> ScriptParser::readInputFilePatterns() {<br>
+  std::vector<Regex> V;<br>
   while (!Error && !skip(")"))<br>
-    V.push_back(next());<br>
+    V.push_back(toRegex(next()));<br>
   return V;<br>
 }<br>
<br>
----------------<br>
ruiu wrote:<br>
> 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.<br>
Ok, I see.<br>
Found one more problem with that unfortunatelly:<br>
<br>
I can't just switch current:<br>
  std::vector<llvm::Regex> ExcludedFiles;<br>
  std::vector<llvm::Regex> SectionPatterns;<br>
<br>
to<br>
<br>
llvm::Regex ExcludedFiles;<br>
llvm::Regex SectionPatterns;<br>
<br>
Because as I mantioned above, llvm::Regex does not have constructor with no arguments.<br>
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<br>
either not to use "|" or to make them unique_ptr.<br>
What do you think ?<br>
<br>
================<br>
Comment at: ELF/LinkerScript.h:97<br>
@@ -95,3 +96,3 @@<br>
   static bool classof(const BaseCommand *C);<br>
-  StringRef FilePattern;<br>
+  std::unique_ptr<llvm::Regex> FilePattern;<br>
   SortKind SortOuter = SortNone;<br>
----------------<br>
ruiu wrote:<br>
> Why unique_ptr?<br>
In compare with std::regex, llvm::Regex does not have constructor with no arguments.<br></blockquote><div><br></div><div>Optional<Regex> is probably more appropriate than unique_ptr, if the need to defer construction is the motivation here.<br><br>(but, yes, probably fine to just have a default ctor for Regex too - but I haven't looked closely to see what the ramifications of that are)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Removed unique_ptr, added argument for InputSectionDescription instead.<br>
<br>
================<br>
Comment at: ELF/LinkerScript.h:132<br>
@@ -130,3 +131,3 @@<br>
   // be kept even if they are unused and --gc-sections is specified.<br>
-  std::vector<StringRef> KeptSections;<br>
+  std::vector<llvm::Regex *> KeptSections;<br>
 };<br>
----------------<br>
ruiu wrote:<br>
> Why pointers?<br>
Restriction of API. llvm::Regex is non-copyable.<br>
That looks to be intended:<br>
 r198334 "Make llvm::Regex non-copyable but movable."<br>
<br>
So I cant just copy kept patterns into KeptSections.<br>
<br>
<br>
================<br>
Comment at: ELF/LinkerScript.h:106<br>
@@ -103,1 +105,3 @@<br>
+  std::vector<llvm::Regex> ExcludedFiles;<br>
+  std::vector<llvm::Regex> SectionPatterns;<br>
 };<br>
----------------<br>
Not sure, do we want to rename SectionPatterns and FilePattern to something then ?<br>
<br>
<br>
<a href="https://reviews.llvm.org/D23829" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23829</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>