Dear All, Based on the few code review comments that I received recently, I do think that I should re-visit basic design that I had proposed initially, and I want to make sure that the community is in agreement with it, before I proceed further.
The very first design question to be discussed is about *scanning/parsing* OpenMP names. As you know, there are so many OpenMP specific names like parallel, for, private, shared, etc. And, following are two possible choices to scan/parse them. 1. Treat OpenMP names as *keywords*, introduce new lexical tokens for each one them, make changes to Lexer/Parser to scan them as either OpenMP keywords or as identifiers or as C/C++ keywords depending on the context. 2. Store all OpenMP names in a string table, and while parsing OpenMP directive statements, perform string search and comparison in order to parse them as either valid OpenMP names (upon successful string search) or report syntax errors (upon unsuccessful string search). [Any other possible choices?] Both the above approaches have their pros and cons. First one looks elegant at least theoretically, but requires changes to some of the performance critical pieces of Lexer component, and handling of identifier token will become very tricky. Second one does not look more elegant as per compiler theory and requires heavy string search and comparisons, but, avoids changes to Lexer component, and all subsequent trickiness involved for handling identifier tokens. I preferred second approach since Clang parser is recursive descent in nature, mentioned the same in the proposal document, and proceeded further since there was no any remark on my approach from any one. But, now, based on few other code review comments, which in turn have root in my original approach, I would like to open it again, discuss it, and come to a common conclusion before proceeding further. -- mahesha On Sat, Oct 27, 2012 at 9:21 PM, Mahesha HS <[email protected]> wrote: > 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: >> > > Hi Chandler, > >> 1) The over-use of OwningPtr seems like a serious code smell. I would >> want to understand why these are pointers at all here. > > In general, any pragma support in Clang requires it to be first > registered with Preprocessor. The way to register it is by deriving a > new class from pragma base class interface - "class Pragma", and then > by adding an instance of the derived class to Preprocessor's pragma > handler object. Since OpenMP has around *fifteen* different pragma > directives, we need a separate instance for each of them. So, there > seems to be a over-use of OwningPtr. > > However, as I am new to Clang code base, my current usual practice is > to look into exiting relevant code base and try to follow the same > approach when it comes to allocating new objects, conservative > implementation, etc. I looked into the Parser class, where already > OwningPtr is used liberally to some extent to support few pragmas. So, > I thought of following the same approach. > > However, your comment is well taken. I will see if I can do it more > efficient way. I also welcome your suggestions in this regard. > >> 2) There are copious "doxygen" formatted comments that don't use a >> doxygen comment prefix of '///', instead using the normal '//'. > > I will be very careful with these violations. > >> 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. > > If possible, please give me some hints so that it helps me to create > more robust test cases. This is very crucial at this point since I am > still new to Clang environment. > >> 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. > > By default, OpenMP feature is *off* in all most all compilers. Only > when the user passes -fopenmp, it will be turned *on*. So, I did not > think about -fno-openmp. However, I will think about it, and support > it too, if we decide that it will be useful. > > -- > mahesha -- mahesha _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
