[PATCH] D23829: [ELF] - Use std::regex instead of hand written logic in elf::globMatch()
George Rimar via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 31 04:48:10 PDT 2016
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
More information about the llvm-commits
mailing list