[PATCH] Change OwningPtr::take() to OwningPtr::release().

[Manuel Klimek]

On Fri, Mar 7, 2014 at 7:58 PM, Ahmed Charles <acharles at outlook.com> wrote:

>
>   Thanks.
>
>
> ================
> Comment at: include/clang/Frontend/FrontendAction.h:149
> @@ -148,5 +148,3 @@
>
> -  ASTUnit *takeCurrentASTUnit() {
> -    return CurrentASTUnit.take();
> -  }
> +  ASTUnit *takeCurrentASTUnit() { return CurrentASTUnit.release(); }
>
> ----------------
> David Blaikie wrote:
> > I think clang-format might now have a mode in which it doesn't reformat
> lines you didn't edit (so this would remain on 3 lines instead of being
> clumped into one)...
> >
> > But I'm not too fussed - just a possible FYI for this or next time in
> case you endu p dealing with more complicated patches that would benefit
> from easier diffs.
> I don't see that as a command-line flag. It does refuse to make changes to
> certain things which are on lines that weren't edited, but it also does
> things like this. In any case, I went through each edit and decided they
> were fine given the complexity of the actual change going on and hopefully
> it will make future changes have nicer diffs.
>
> ================
> Comment at: lib/CodeGen/ModuleBuilder.cpp:51
> @@ -50,5 +50,3 @@
>
> -    virtual llvm::Module* ReleaseModule() {
> -      return M.take();
> -    }
> +    virtual llvm::Module *ReleaseModule() { return M.release(); }
>
> ----------------
> David Blaikie wrote:
> > Feel free to commit cleanup (like the moved '*' or the move to one-line
> - if you aren't using the "only modify lines I touched" thing I mentioned
> earlier - or if I hallucinated and that doesn't really exist) ahead of time
> without precommit review if you like - though it's not a big deal on a
> patch like this that's been manufactured by replacements and clang-format.
> Interestingly enough, the first diff is without clang-format running and
> the second is after it was run, so you could always just look at the first
> one, which would have the same effect, at least for pre-review purposes.
>
> But agreed, that would be nicer for more complex changes.
>
> ================
> Comment at: lib/Lex/PTHLexer.cpp:539
> @@ -540,1 +538,3 @@
> +                        SL.release(), NumIds, spellingBase,
> +                        (const char *)originalSourceBase);
>  }
> ----------------
> David Blaikie wrote:
> > More orthogonal cleanup - might be worth filing bugs on
> clang-format-patch (or git-clang-format, or whatever other tools) for these.
> I'm not sure whether it's a bug or by design. The vim integration code for
> clang-format says: The line or region is extended to the next bigger
> syntactic entity.
>
> This suggests that this is possibly a feature. I'm not sure what it would
> mean to reflow the first two lines without ever changing the third in this
> scenario, for example, if adding release make it hit the 80-col limit
> twice, it would obviously be better to change the third line rather than
> insert a new one. But in making that work correctly, you end up with
> orthogonal changes happening as well.
>
> In any case, it seems like clang-format isn't designed to simply ignore
> existing formatting, for example, there's no option to have it not touch
> the following code:
>
> `#include <foo> // comment`
> `#include <barbarbar> // comment`
> `#include <baz>       // comment`
>
> It will either change the indentation of the comment on line 1 or line 3,
> but no matter what you do, it will change one of them.
>

clang-format should (if used with the right scripts) only re-format
statements around touched lines (it will mostly re-format full statements
though).

What scripts did you use to clang-format the changes?

Cheers,
/Manuel


>
> http://llvm-reviews.chandlerc.com/D3000
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140309/9a49a58b/attachment.html>