Just one comment there. Otherwise looks good. http://codereview.appspot.com/4707044/diff/1/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4707044/diff/1/lib/Sema/SemaDeclAttr.cpp#newcod... lib/Sema/SemaDeclAttr.cpp:211: /// \return true if the number of argumenation units does not match expectation This comment doesn't appear to line up with the actual return value from what I can see.
This is the right version of the patch for me to be reviewing? Is there any other patches prior to this one you have out for review? Most of the comments are cosmetic. A few substantive ones. Should be good to mail upstream once this is a bit closer to the coding conventions. http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html File docs/LanguageExtensions.html (right): http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1125: <h4 id="ts_noanal">no_thread_safety_analysis</h4> given a namespace, would 'skip_analysis' be a better name? http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1159: specify that the variable must be accessed while holding lock l.</p> <tt>l</tt> here and below. http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1164: specify that the pointer must be dereferenced while holding lock l.</p> This implies that the above attribute needs some important clarifications: when annotating a pointer, is it the pointer that is guarded or the pointed-to-value? I would intuitively expect the former, but this attribute makes me feel it is in fact the latter! Is this really the way these today work?? http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1185: arguments: either of lockable type or integers indexing into Do we want to hint at more user-friendly options than function parameter indexing with integers? Specifically that we plan to add attributes directly on the function parameters when that is well defined (C++0x attributes)? http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1193: the locks may be shared (e.g. read locks). From here on down I feel like several of the paragraphs need to be re-flowed... http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:30: enum AttributeDeclType { This should be a '...Kind' rather than '...Type'. http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:301: bool pointer = false) { indent is off http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:381: bool exclusive = false) { indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:384: // zero or more arguments ok ? Not sure why this is here http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:415: S.Context)); indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:423: bool exclusive = false) { indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:437: S.Context)); indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:440: S.Context)); indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:445: const AttributeList &Attr) { indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:448: // zero or more arguments ok No need for comment. http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:460: const AttributeList &Attr) { indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:476: const AttributeList &Attr) { indent http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety.cpp (right): http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:6: */ Why do we have a new comment system here? These are tests. We don't need this much documentation. http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:16: ***********************************/ If you're going to use banner comments, use an existing banner comment pattern in LLVM/Clang. That said, I don't think they add anything. http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:261: //2.Deal with argument parsing: This should either not be here, or have a FIXME attached and likely use #if 0
http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html File docs/LanguageExtensions.html (right): http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1125: <h4 id="ts_noanal">no_thread_safety_analysis</h4> On 2011/08/01 18:23:53, chandlerc wrote: > given a namespace, would 'skip_analysis' be a better name? Are we for-sure transitioning to a namespace? Or is this still under discussion? When I initially floated the idea a month or so ago you convinced me to not use a namespace for now... http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1159: specify that the variable must be accessed while holding lock l.</p> Fixed On 2011/08/01 18:23:53, chandlerc wrote: > <tt>l</tt> here and below. http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1164: specify that the pointer must be dereferenced while holding lock l.</p> It is the latter. To guard the pointer, you annotate with guarded_by. However, the current gcc impl does not do any fancy pointer analysis -- annotating a pointer with pt_guarded_by(z) just means that at the time the pointer is dereferenced you must be holding z. On 2011/08/01 18:23:53, chandlerc wrote: > This implies that the above attribute needs some important clarifications: when > annotating a pointer, is it the pointer that is guarded or the pointed-to-value? > > I would intuitively expect the former, but this attribute makes me feel it is in > fact the latter! Is this really the way these today work?? http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1185: arguments: either of lockable type or integers indexing into Yes. I think the implementation of annotalysis does also allow using the function parameters explicitly as arguments (which are arguments of lockable type). On 2011/08/01 18:23:53, chandlerc wrote: > Do we want to hint at more user-friendly options than function parameter > indexing with integers? Specifically that we plan to add attributes directly on > the function parameters when that is well defined (C++0x attributes)? http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#... docs/LanguageExtensions.html:1193: the locks may be shared (e.g. read locks). Fixed On 2011/08/01 18:23:53, chandlerc wrote: > From here on down I feel like several of the paragraphs need to be re-flowed... http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:30: enum AttributeDeclType { I just went back to an anonymous name. On 2011/08/01 18:23:53, chandlerc wrote: > This should be a '...Kind' rather than '...Type'. http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:301: bool pointer = false) { fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent is off http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:381: bool exclusive = false) { Fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:384: // zero or more arguments ok Each function for handling an attribute has similar structure, starting with checking to make sure the number of arguments matches. This does not need such a check since zero or more args are ok. So this is documenting that fact so that it is clear the num args check was not just forgotten. On 2011/08/01 18:23:53, chandlerc wrote: > ? Not sure why this is here http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:415: S.Context)); fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:423: bool exclusive = false) { fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:437: S.Context)); fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:440: S.Context)); fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:445: const AttributeList &Attr) { fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:448: // zero or more arguments ok I would prefer to keep it. See above discussion. On 2011/08/01 18:23:53, chandlerc wrote: > No need for comment. http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:460: const AttributeList &Attr) { fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:476: const AttributeList &Attr) { fixed On 2011/08/01 18:23:53, chandlerc wrote: > indent http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety.cpp (right): http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:6: */ Changed to //-----------------------------------------// banner system. On 2011/08/01 18:23:53, chandlerc wrote: > Why do we have a new comment system here? These are tests. We don't need this > much documentation. http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:16: ***********************************/ Changed to //-----------------------------------------// banner system. They make it much easier for me to browse/edit the test file. On 2011/08/01 18:23:53, chandlerc wrote: > If you're going to use banner comments, use an existing banner comment pattern > in LLVM/Clang. > > That said, I don't think they add anything. http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety.cpp:261: //2.Deal with argument parsing: Already replaced by the next patch. On 2011/08/01 18:23:53, chandlerc wrote: > This should either not be here, or have a FIXME attached and likely use #if 0