Hi Chandler, I reverted back all the changes. I am sorry. I was in an impression that code is in a good shape to commit as it had went through few rounds of review. Also, I had misread the line "Code can be reviewed either before it is committed or after" from http://llvm.org/docs/DeveloperPolicy.html
Thanks for your review comments. I will go through them. And, I will wait till I get a formal approval. -- mahesha On Sat, Oct 27, 2012 at 2:48 PM, Chandler Carruth <[email protected]> wrote: > On Sat, Oct 27, 2012 at 2:10 AM, Chandler Carruth <[email protected]> > wrote: >> On Fri, Oct 26, 2012 at 11:14 PM, Mahesha HS <[email protected]> wrote: >>> Hi Dmitri, >>> >>> Thanks for the review. I have taken care of all your comments. With >>> respect to the use of std::lower_bound() instead of our own binary >>> search implementation, looks like, it is *not* straightforward as the >>> search algorithm actually need to operate on a map table which holds >>> the pair <StringRef, Enum Type>. However, I will re-think about it and >>> modify it if it is possible in some way. >>> >>> I will be committing first three patches as most of the review >>> comments are taken care for them. I will cross verify and make sure >>> that these patches will *strictly* adhere to CLANG/LLVM coding >>> standard before committing. >> >> I'm really sorry, but that is *not* the policy of pre-commit review, >> or commit access after *approval*. >> >> Please wait until you get explicit approval, especially for such very >> large feature additions to Clang. Reviewers often focus on one set of >> issues for a particular round of review and may have further issues >> they need you to address before committing. > > I see you have already committed all three without waiting for > approval. Please don't abuse the commit privilege in this way. > > The code still has some demonstrable problems by the way, which I > found with only a casual glance. I would assume that these would have > come up in subsequent review: > > 1) The over-use of OwningPtr seems like a serious code smell. I would > want to understand why these are pointers at all here. > 2) There are copious "doxygen" formatted comments that don't use a > doxygen comment prefix of '///', instead using the normal '//'. > 3) The test cases seem quite haphazard; these are often the last > things to be addressed in code review, but possibly I'm just skimming > the patch in too little detail and much of the testing will come > later. > 4) There is no -fno-openmp to complement -fopenmp. Having this ability > to turn off features by appending flags is something we strive for in > the driver. -- mahesha _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
