[cfe-commits] Threadsafety-- basic lockset implementation

[Caitlin Sadowski]

Ted,

You are right -- they were pretty munged together.

Here is a three patch sequence that first refactors
AnalysisBasedWarnings to use an error handler, then refactors that
handler slightly (changing some tests), then moves the analysis to a
separate file. Is this better? Any other comments, or am I good to
submit?

Cheers,

Caitlin

On Thu, Sep 8, 2011 at 11:03 AM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Caitlin,
>
> This looks good, but I have a few comments (inline, below).  In general, please try and separate functionality changes from refactoring changes.  Specifically, moving the code out of AnalysisBasedWarnings.cpp should be a separate patch from changing any of the wording of the warnings, test cases, and so forth.  The idea is that if the tests don't change, it's clear that there is no functionality change as well.  Since the changes are intermingled, if anything breaks its hard to tell if it was the refactoring that broke things or any other functionality change.
>
> Some nits below:
>
> ++ b/include/clang/Analysis/Analyses/ThreadSafety.h
> @@ -0,0 +1,71 @@
> +//*** ThreadSafety.h -----------------------------------------*- C++ --*-===//
>
> This is really minor, but "***" doesn't start the preamble header of any other header.  Please use "===", like everywhere else.
>
> --- a/lib/Analysis/CMakeLists.txt
> +++ b/lib/Analysis/CMakeLists.txt
> @@ -14,6 +14,7 @@ add_clang_library(clangAnalysis
>   ReachableCode.cpp
>   ScanfFormatString.cpp
>   UninitializedValues.cpp
> +  ThreadSafety.cpp
>   )
>
> Please keep this list alphabetized.
>
> The rest looks fine to me.
>
> Ted
>
> On Sep 7, 2011, at 12:58 PM, Caitlin Sadowski wrote:
>
>> Ted (and whoever else is interested),
>>
>> Here is a patch that moves the analysis to a separate file and
>> decouples the error reporting code. Let me know if this is what you
>> had in mind.
>>
>> This patch is also available for review at:
>> http://codereview.appspot.com/4973067/
>>
>> In this review, the moved code is also left in
>> AnalysisBasedWarnings.cpp so as to make it easier to see what I have
>> changed along with the physical move:
>> http://codereview.appspot.com/4988045/
>>
>> Cheers,
>>
>> Caitlin
>>
>> On Tue, Aug 23, 2011 at 4:46 PM, Ted Kremenek <kremenek at apple.com> wrote:
>>>
>>> On Aug 23, 2011, at 4:31 PM, Caitlin Sadowski wrote:
>>>
>>> This checker is also getting pretty big.  Generally speaking
>>> AnalysisBasedWarnings.cpp should only contain the "top-level" logic for
>>> driving analyses.  The analyses themselves should be in a separate file and
>>> activated via a small API.  When you get a chance, please move the bulk of
>>> the logic to libAnalysis, and have AnalysisBasedWarnings.cpp just call the
>>> entry point.
>>>
>>> Ok, sounds good! I will do this soon.
>>>
>>> Thanks so much!
>>> The silver lining of moving it to libAnalysis is that it cleanly decouples
>>> the policy of issuing warnings from the analysis itself.  That allows the
>>> analysis to be (potentially) reused in different contexts.
>> <threadsafety_separatefile.patch>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafety_filemove0.patch
Type: text/x-patch
Size: 26016 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110908/b672f256/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafety_filemove1.patch
Type: text/x-patch
Size: 5120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110908/b672f256/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafety_filemove2.patch
Type: text/x-patch
Size: 68879 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110908/b672f256/attachment-0002.bin>