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

[David Blaikie via llvm-commits]

On Wed, Aug 31, 2016 at 4:48 AM George Rimar via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> 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.
>

Optional<Regex> is probably more appropriate than unique_ptr, if the need
to defer construction is the motivation here.

(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)


> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160831/4ac52452/attachment.html>