> On Oct 30, 2017, at 8:40 AM, David Blaikie wrote:
>
>
>
> On Mon, Oct 23, 2017 at 5:01 PM George Karpenkov via Phabricator via
> cfe-commits >
> wrote:
> george.karpenkov added a comment.
>
> @dcoughlin
On Mon, Oct 23, 2017 at 5:01 PM George Karpenkov via Phabricator via
cfe-commits wrote:
> george.karpenkov added a comment.
>
> @dcoughlin the context I was thinking about is that if everyone
> consistently runs `clang-format` (if we want that), then we never would
>
george.karpenkov added inline comments.
Comment at: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp:607
+AnalysisDeclContextManager::~AnalysisDeclContextManager() {
+ if (!BdyFrm)
+delete BdyFrm;
alexfh wrote:
> This is an almost guaranteed memory leak. I
alexfh added inline comments.
Comment at: cfe/trunk/lib/Analysis/AnalysisDeclContext.cpp:607
+AnalysisDeclContextManager::~AnalysisDeclContextManager() {
+ if (!BdyFrm)
+delete BdyFrm;
This is an almost guaranteed memory leak. I think, you meant `if
george.karpenkov added a comment.
@dcoughlin OK sorry, I haven't considered the point that codebase predates the
requirement, and the master format-all commit was never done.
I've committed this one already, but I will be more careful with applying
clang-format to future changes.
Repository:
dcoughlin added a comment.
That would require going into the past and requiring everyone back then to run
clang-format as well. Unfortunately they didn't -- so human judgment is needed
when modifying code that doesn't obey the guidelines.
Repository:
rL LLVM
https://reviews.llvm.org/D39208
This revision was automatically updated to reflect the committed changes.
Closed by commit rL316400: [Analyzer] Do not use static storage to for
implementations created in BodyFarm. (authored by george.karpenkov).
Changed prior to commit:
https://reviews.llvm.org/D39208?vs=119949=119974#toc
george.karpenkov added a comment.
@dcoughlin the context I was thinking about is that if everyone consistently
runs `clang-format` (if we want that), then we never would have discussion.
The alternative is that every run of `clang-format` would be followed by
manually reverting changes which
dcoughlin added a comment.
I think a good strategy is to look at your diffs and consider whether the
benefits of normalizing the style outweigh the cost of losing the history. In
this case, I think keeping the history makes sense. (Imagine you are a future
maintainer and want to know when and
george.karpenkov added a comment.
@dcoughlin that's not me, that's clang-format.
If we agree on always running it, I think the changes should stay there.
https://reviews.llvm.org/D39208
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Content-wise, LGTM. There is a style nit inline.
Also, can you avoid reformatting the lines that haven't changed? This will help
preserve the history of the file and make it clear what
george.karpenkov created this revision.
Herald added subscribers: szepet, kristof.beyls, xazax.hun, javed.absar,
aemerson.
https://reviews.llvm.org/D39208
Files:
include/clang/Analysis/AnalysisDeclContext.h
include/clang/Analysis/BodyFarm.h
lib/Analysis/AnalysisDeclContext.cpp
12 matches
Mail list logo