You're okay to submit. I will probably move the declarations around slightly in a separate patch. :-)
-DeLesley On Tue, Sep 2, 2014 at 2:19 PM, Ed Schouten <[email protected]> wrote: > Hi Delesley, Aaron, > > On 2 September 2014 22:44, Delesley Hutchins <[email protected]> wrote: > > (1) FactEntry needs a virtual destructor to avoid memory leaks. > > Done! > > > (2) I would like the different FactEntry classes to be declared together > (at > > the top of the file), rather than split as they are currently. > > The problem is that the *LockableFactEntry classes depend on FactSet > and FactManager, whereas the FactSet and FactManager depend on the > FactEntry. > > I could possibly declare them together, but in that case, the > implementations of handle*() would need to go elsewhere. I'm usually > not a big fan of that approach. Be sure to let me know if you'd prefer > that anyway. > > On 2 September 2014 22:49, Aaron Ballman <[email protected]> wrote: > > I would not reformat the entire file at this point (that tends to be > > pretty noisy for little gain). Instead, you can run clang-format over > > the patch file itself. There's a pretty simple script for doing this > > described in the documentation: > > http://clang.llvm.org/docs/ClangFormat.html > > That seems to work perfectly. I've just reformatted the code. Updated > patch: > > http://80386.nl/pub/20140902-clang-scoped_lockable-2.txt > > Be sure to let me know if there's anything else that you'd like to see > differently. I'll probably push in the change some time tomorrow > morning/afternoon. > > Thanks, > -- > Ed Schouten <[email protected]> > -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
