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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
