Re: [PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-30 Thread George Karpenkov via cfe-commits
> 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

Re: [PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-30 Thread David Blaikie via cfe-commits
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 >

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-24 Thread George Karpenkov via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-24 Thread Alexander Kornienko via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread George Karpenkov via Phabricator via cfe-commits
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:

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread Devin Coughlin via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread George Karpenkov via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread George Karpenkov via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread Devin Coughlin via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread George Karpenkov via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread Devin Coughlin via Phabricator via cfe-commits
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

[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp

2017-10-23 Thread George Karpenkov via Phabricator via cfe-commits
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