Hi Matthieu, Thanks a lot. It was a very useful information about Clang/LLVM check-in policy.
-- mahesha On Sat, Oct 27, 2012 at 4:51 PM, Matthieu Monrocq <[email protected]> wrote: > Hi Mahesha, > > I understand it may be difficult to get how things are working here, so let > me explain what I have understood of it (so far). Do mind that I am mostly a > lurker here so my words may be slightly off. > > There are two categories of developers working on Clang: those who are > trusted and those who are not trusted *yet*. Clang is a large body of code, > with many areas, so it will take time before a newcomer is comfortable in > coding there and understand the implications of his work, and he/she will > probably not be trusted before this happens. > > In general, those who are trusted will commit and their patch will get > reviewed after the fact; unless they want to consult their peers and > voluntarily submit themselves to a pre-commit review to gather opinions > (generally on the cfe-dev list). > > Those who are not trusted yet should ask for approval before committing > every time for non-trivial patches (trivial patches are for example > typo/grammaro corrections); this approval will be formal (sometimes > abbreviated to LGTM, other times more explicit). > > Being new, you are not trusted yet and so will need to get approval from the > code owner of the area you are working on. This approval will only be given > after one or several reviews, please do not be afraid of them, it is > expected that it takes time to learn new coding guidelines *and* new idioms > *and* the organization and goals of each piece! > > Because reviewing takes so much time, many people may participate, but only > the code owner of the area you have been working on may deliver the final > go-ahead. > > Finally, the last round may not be completely "spotless", and you may get a > comment such as "with those fixed, LGTM" which indicates that the code is > good to go after the few fixes evoked in above. This should be the only > incidence where you can commit a patch without further approval. > > Hope this helps. > > -- Matthieu > > On Sat, Oct 27, 2012 at 1:00 PM, Mahesha HS <[email protected]> wrote: >> >> 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 > > -- mahesha _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
