[cfe-commits] Threadsafety-- basic lockset implementation
Ted Kremenek
kremenek at apple.com
Thu Sep 8 11:03:48 PDT 2011
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>
More information about the cfe-commits
mailing list