Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1541063002:
misc cleanup of Compiler::run (Closed)
|
Created:
5 years ago by rkotlerimgtec Modified:
4 years, 12 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionmisc cleanup of Compiler::run
This assumes patch from 1534883005 though still needs to rename
validateAndGenerateBuildAttributes to dumpBuildAttributes
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=93d85ce0a3d63a9bd40973212225bb9513f1d170
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase to master #
Total comments: 2
Patch Set 3 : changes suggested by stichnot #
Total comments: 2
Patch Set 4 : changes suggested by stichnot #Messages
Total messages: 18 (4 generated)
Description was changed from ========== misc cleanup of Compiler::run This assumes patch from 1534883005 though still needs to rename validateAndGenerateBuildAttributes to dumpBuildAttributes BUG= ========== to ========== misc cleanup of Compiler::run This assumes patch from 1534883005 though still needs to rename validateAndGenerateBuildAttributes to dumpBuildAttributes BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
Rebase to master branch. Note, this patch also fixes several places where a value is returned in a method declared as returning void.
Rebase to master branch. Note, this patch also fixes several places where a value is returned in a method declared as returning void.
https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, std::regex(".*\\.ll")) && I would also be okay to just create a function where IceString is declared that examines the last part of the string. I used regex here just because I could do all of this on one line. I prefer to think of what is on the end of the string as a low level operation and not in the same place where more high level considerations are at play.
https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 src/IceCompiler.cpp:20: #include <regex> Our (unwritten) convention is to put system includes last. https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, std::regex(".*\\.ll")) && On 2015/12/22 23:21:55, rkotlerimgtec wrote: > I would also be okay to just create a function where IceString is declared that > examines the last part of the string. I used regex here just because I could do > all of this on one line. > > I prefer to think of what is on the end of the string as a low level operation > and not in the same place where more high level considerations are at play. > The regex use is nice. However, I would like it to not be used in a MINIMAL build, because the regex code increases the translator size and textual .ll file parsing is disabled in MINIMAL mode. Your change removed the BuildDefs::llvmIrAsInput() check which had accomplished this. This led me down a path of trying to make sense of llvmIr() and llvmIrAsInput() and the --build-on-read argument. I think these could be simplified. The logic could be: if some BuildDefs flag is defined, and the input file name ends in ".ll", then use the LLVM parser, else use the Subzero parser. That is probably best left to a separate CL, but in the meantime it would be best to retain the BuildDefs::llvmIrAsInput() check.
I"m not sure what you want here. Do you want to abandon this whole patch for now? Or just get rid of the regex? On Wed, Dec 23, 2015 at 7:54 AM, <stichnot@chromium.org> wrote: > > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp > File src/IceCompiler.cpp (right): > > > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 > src/IceCompiler.cpp:20: #include <regex> > Our (unwritten) convention is to put system includes last. > > https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp > File src/IceCompiler.cpp (right): > > > https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... > src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, > std::regex(".*\\.ll")) && > On 2015/12/22 23:21:55, rkotlerimgtec wrote: > >> I would also be okay to just create a function where IceString is >> > declared that > >> examines the last part of the string. I used regex here just because I >> > could do > >> all of this on one line. >> > > I prefer to think of what is on the end of the string as a low level >> > operation > >> and not in the same place where more high level considerations are at >> > play. > > > The regex use is nice. However, I would like it to not be used in a > MINIMAL build, because the regex code increases the translator size and > textual .ll file parsing is disabled in MINIMAL mode. Your change > removed the BuildDefs::llvmIrAsInput() check which had accomplished > this. > > This led me down a path of trying to make sense of llvmIr() and > llvmIrAsInput() and the --build-on-read argument. I think these could > be simplified. The logic could be: if some BuildDefs flag is defined, > and the input file name ends in ".ll", then use the LLVM parser, else > use the Subzero parser. > > That is probably best left to a separate CL, but in the meantime it > would be best to retain the BuildDefs::llvmIrAsInput() check. > > https://codereview.chromium.org/1541063002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 2015/12/23 18:57:54, rkotlerimgtec wrote: > I"m not sure what you want here. Do you want to abandon this whole patch > for now? > Or just get rid of the regex? > > On Wed, Dec 23, 2015 at 7:54 AM, <mailto:stichnot@chromium.org> wrote: > > > > > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp > > File src/IceCompiler.cpp (right): > > > > > > > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 > > src/IceCompiler.cpp:20: #include <regex> > > Our (unwritten) convention is to put system includes last. > > > > https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp > > File src/IceCompiler.cpp (right): > > > > > > > https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... > > src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, > > std::regex(".*\\.ll")) && > > On 2015/12/22 23:21:55, rkotlerimgtec wrote: > > > >> I would also be okay to just create a function where IceString is > >> > > declared that > > > >> examines the last part of the string. I used regex here just because I > >> > > could do > > > >> all of this on one line. > >> > > > > I prefer to think of what is on the end of the string as a low level > >> > > operation > > > >> and not in the same place where more high level considerations are at > >> > > play. > > > > > > The regex use is nice. However, I would like it to not be used in a > > MINIMAL build, because the regex code increases the translator size and > > textual .ll file parsing is disabled in MINIMAL mode. Your change > > removed the BuildDefs::llvmIrAsInput() check which had accomplished > > this. > > > > This led me down a path of trying to make sense of llvmIr() and > > llvmIrAsInput() and the --build-on-read argument. I think these could > > be simplified. The logic could be: if some BuildDefs flag is defined, > > and the input file name ends in ".ll", then use the LLVM parser, else > > use the Subzero parser. > > > > That is probably best left to a separate CL, but in the meantime it > > would be best to retain the BuildDefs::llvmIrAsInput() check. > > > > https://codereview.chromium.org/1541063002/ > > Regex is fine. Just please make sure to test against BuildDefs::llvmIrAsInput() as the original code does. Something like this: diff --git a/src/IceCompiler.cpp b/src/IceCompiler.cpp index 349990c..1eeef6b 100644 --- a/src/IceCompiler.cpp +++ b/src/IceCompiler.cpp @@ -99,8 +99,9 @@ void Compiler::run(const Ice::ClFlagsExtra &ExtraFlags, GlobalContext &Ctx, std::unique_ptr<Translator> Translator; const IceString &IRFilename = ExtraFlags.getIRFilename(); // Force -build-on-read=0 for .ll files. - if (!std::regex_match(IRFilename, std::regex(".*\\.ll")) && - ExtraFlags.getBuildOnRead()) { + if (ExtraFlags.getBuildOnRead() && + !(BuildDefs::llvmIrAsInput() && + std::regex_match(IRFilename, std::regex(".*\\.ll")))) { std::unique_ptr<PNaClTranslator> PTranslator(new PNaClTranslator(&Ctx)); std::unique_ptr<llvm::StreamingMemoryObject> MemObj( new llvm::StreamingMemoryObjectImpl(InputStream.release())); though I feel like that expression is hard to get one's head around, so maybe the original structure with the local BuildOnRead variable would be clearer.
Gotcha. I was careless there. Oops. I will fix that. On Wed, Dec 23, 2015 at 1:28 PM, <stichnot@chromium.org> wrote: > On 2015/12/23 18:57:54, rkotlerimgtec wrote: > >> I"m not sure what you want here. Do you want to abandon this whole patch >> for now? >> Or just get rid of the regex? >> > > On Wed, Dec 23, 2015 at 7:54 AM, <mailto:stichnot@chromium.org> wrote: >> > > > >> > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp >> > File src/IceCompiler.cpp (right): >> > >> > >> > >> > > > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 > >> > src/IceCompiler.cpp:20: #include <regex> >> > Our (unwritten) convention is to put system includes last. >> > >> > >> https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp >> > File src/IceCompiler.cpp (right): >> > >> > >> > >> > > > https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... > >> > src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, >> > std::regex(".*\\.ll")) && >> > On 2015/12/22 23:21:55, rkotlerimgtec wrote: >> > >> >> I would also be okay to just create a function where IceString is >> >> >> > declared that >> > >> >> examines the last part of the string. I used regex here just because I >> >> >> > could do >> > >> >> all of this on one line. >> >> >> > >> > I prefer to think of what is on the end of the string as a low level >> >> >> > operation >> > >> >> and not in the same place where more high level considerations are at >> >> >> > play. >> > >> > >> > The regex use is nice. However, I would like it to not be used in a >> > MINIMAL build, because the regex code increases the translator size and >> > textual .ll file parsing is disabled in MINIMAL mode. Your change >> > removed the BuildDefs::llvmIrAsInput() check which had accomplished >> > this. >> > >> > This led me down a path of trying to make sense of llvmIr() and >> > llvmIrAsInput() and the --build-on-read argument. I think these could >> > be simplified. The logic could be: if some BuildDefs flag is defined, >> > and the input file name ends in ".ll", then use the LLVM parser, else >> > use the Subzero parser. >> > >> > That is probably best left to a separate CL, but in the meantime it >> > would be best to retain the BuildDefs::llvmIrAsInput() check. >> > >> > https://codereview.chromium.org/1541063002/ >> > >> > > Regex is fine. Just please make sure to test against > BuildDefs::llvmIrAsInput() > as the original code does. Something like this: > > diff --git a/src/IceCompiler.cpp b/src/IceCompiler.cpp > index 349990c..1eeef6b 100644 > --- a/src/IceCompiler.cpp > +++ b/src/IceCompiler.cpp > @@ -99,8 +99,9 @@ void Compiler::run(const Ice::ClFlagsExtra &ExtraFlags, > GlobalContext &Ctx, > std::unique_ptr<Translator> Translator; > const IceString &IRFilename = ExtraFlags.getIRFilename(); > // Force -build-on-read=0 for .ll files. > - if (!std::regex_match(IRFilename, std::regex(".*\\.ll")) && > - ExtraFlags.getBuildOnRead()) { > + if (ExtraFlags.getBuildOnRead() && > + !(BuildDefs::llvmIrAsInput() && > + std::regex_match(IRFilename, std::regex(".*\\.ll")))) { > std::unique_ptr<PNaClTranslator> PTranslator(new > PNaClTranslator(&Ctx)); > std::unique_ptr<llvm::StreamingMemoryObject> MemObj( > new llvm::StreamingMemoryObjectImpl(InputStream.release())); > > though I feel like that expression is hard to get one's head around, so > maybe > the original structure with the local BuildOnRead variable would be > clearer. > > https://codereview.chromium.org/1541063002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
I will look at a few variants to make it read the best. In general I don't like having lots of low level details like how to parse the end of a filename string mixed in with more general considerations. I like for the code to simply read that we are testing the condition of the filename extension. Regex was just a compromise to avoid creating a separate function but the separate function would read better. In Boost and some other libraries they have specific string tests for the end of a string and such but STL does not have it. I also try and avoid repeating information. You can be the judge as to whether in the end things are improved. I'm a stickler for doing this and to many I'm excessive about this but it really helps me when I'm reading code when it looks like that. I'd prefer to just move the original non regex code somewhere else even just to make it read better and have a function that tests a filename. On Wed, Dec 23, 2015 at 1:58 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > Gotcha. I was careless there. Oops. I will fix that. > > On Wed, Dec 23, 2015 at 1:28 PM, <stichnot@chromium.org> wrote: > >> On 2015/12/23 18:57:54, rkotlerimgtec wrote: >> >>> I"m not sure what you want here. Do you want to abandon this whole patch >>> for now? >>> Or just get rid of the regex? >>> >> >> On Wed, Dec 23, 2015 at 7:54 AM, <mailto:stichnot@chromium.org> wrote: >>> >> >> > >>> > https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp >>> > File src/IceCompiler.cpp (right): >>> > >>> > >>> > >>> >> >> >> https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 >> >>> > src/IceCompiler.cpp:20: #include <regex> >>> > Our (unwritten) convention is to put system includes last. >>> > >>> > >>> https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp >>> > File src/IceCompiler.cpp (right): >>> > >>> > >>> > >>> >> >> >> https://codereview.chromium.org/1541063002/diff/20001/src/IceCompiler.cpp#new... >> >>> > src/IceCompiler.cpp:102: if (!std::regex_match(IRFilename, >>> > std::regex(".*\\.ll")) && >>> > On 2015/12/22 23:21:55, rkotlerimgtec wrote: >>> > >>> >> I would also be okay to just create a function where IceString is >>> >> >>> > declared that >>> > >>> >> examines the last part of the string. I used regex here just because I >>> >> >>> > could do >>> > >>> >> all of this on one line. >>> >> >>> > >>> > I prefer to think of what is on the end of the string as a low level >>> >> >>> > operation >>> > >>> >> and not in the same place where more high level considerations are at >>> >> >>> > play. >>> > >>> > >>> > The regex use is nice. However, I would like it to not be used in a >>> > MINIMAL build, because the regex code increases the translator size and >>> > textual .ll file parsing is disabled in MINIMAL mode. Your change >>> > removed the BuildDefs::llvmIrAsInput() check which had accomplished >>> > this. >>> > >>> > This led me down a path of trying to make sense of llvmIr() and >>> > llvmIrAsInput() and the --build-on-read argument. I think these could >>> > be simplified. The logic could be: if some BuildDefs flag is defined, >>> > and the input file name ends in ".ll", then use the LLVM parser, else >>> > use the Subzero parser. >>> > >>> > That is probably best left to a separate CL, but in the meantime it >>> > would be best to retain the BuildDefs::llvmIrAsInput() check. >>> > >>> > https://codereview.chromium.org/1541063002/ >>> > >>> >> >> Regex is fine. Just please make sure to test against >> BuildDefs::llvmIrAsInput() >> as the original code does. Something like this: >> >> diff --git a/src/IceCompiler.cpp b/src/IceCompiler.cpp >> index 349990c..1eeef6b 100644 >> --- a/src/IceCompiler.cpp >> +++ b/src/IceCompiler.cpp >> @@ -99,8 +99,9 @@ void Compiler::run(const Ice::ClFlagsExtra &ExtraFlags, >> GlobalContext &Ctx, >> std::unique_ptr<Translator> Translator; >> const IceString &IRFilename = ExtraFlags.getIRFilename(); >> // Force -build-on-read=0 for .ll files. >> - if (!std::regex_match(IRFilename, std::regex(".*\\.ll")) && >> - ExtraFlags.getBuildOnRead()) { >> + if (ExtraFlags.getBuildOnRead() && >> + !(BuildDefs::llvmIrAsInput() && >> + std::regex_match(IRFilename, std::regex(".*\\.ll")))) { >> std::unique_ptr<PNaClTranslator> PTranslator(new >> PNaClTranslator(&Ctx)); >> std::unique_ptr<llvm::StreamingMemoryObject> MemObj( >> new llvm::StreamingMemoryObjectImpl(InputStream.release())); >> >> though I feel like that expression is hard to get one's head around, so >> maybe >> the original structure with the local BuildOnRead variable would be >> clearer. >> >> https://codereview.chromium.org/1541063002/ >> > > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 src/IceCompiler.cpp:20: #include <regex> On 2015/12/23 15:54:15, stichnot wrote: > Our (unwritten) convention is to put system includes last. This still needs to be done. https://codereview.chromium.org/1541063002/diff/60001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/60001/src/IceCompiler.cpp#new... src/IceCompiler.cpp:60: if (Stream == nullptr) The change to how this is called is nice. As a result, the Stream==nullptr check is now unnecessary. Better still, pass the stream arg as Ostream &Stream. (Also, the rest of the code names the variable "Ostream &Str" .)
https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/1/src/IceCompiler.cpp#newcode20 src/IceCompiler.cpp:20: #include <regex> On 2015/12/25 15:31:01, stichnot wrote: > On 2015/12/23 15:54:15, stichnot wrote: > > Our (unwritten) convention is to put system includes last. > > This still needs to be done. Done. https://codereview.chromium.org/1541063002/diff/60001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1541063002/diff/60001/src/IceCompiler.cpp#new... src/IceCompiler.cpp:60: if (Stream == nullptr) On 2015/12/25 15:31:01, stichnot wrote: > The change to how this is called is nice. As a result, the Stream==nullptr > check is now unnecessary. Better still, pass the stream arg as Ostream &Stream. > > (Also, the rest of the code names the variable "Ostream &Str" .) Done.
lgtm
Description was changed from ========== misc cleanup of Compiler::run This assumes patch from 1534883005 though still needs to rename validateAndGenerateBuildAttributes to dumpBuildAttributes BUG= ========== to ========== misc cleanup of Compiler::run This assumes patch from 1534883005 though still needs to rename validateAndGenerateBuildAttributes to dumpBuildAttributes BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) manually as 93d85ce0a3d63a9bd40973212225bb9513f1d170 (presubmit successful). |
