HazardyKnusperkeks added inline comments.
================
Comment at: clang/lib/Format/Format.cpp:3486-3489
+ Expanded.InsertBraces = true;
+ Passes.emplace_back([&](const Environment &Env) {
+ return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
});
----------------
MyDeveloperDay wrote:
> owenpan wrote:
> > tahonermann wrote:
> > > How about using an init capture instead? This will suffice to avoid one
> > > of the copies but means that `InsertBraces` doesn't get set until the
> > > lambda is invoked. I wouldn't expect that to matter though.
> > I'm not sure if it's worth the trouble, but if I really had to bother, I
> > would do something like the above.
> Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this
> bascially do the same without us having to remember to put Expanded back
> afterwards? I don't see how using Expanded is any different from using S
> (other than the copy)
>
> ```
> if (Style.InsertBraces) {
> FormatStyle S = Expanded;
> S.InsertBraces = true;
> Passes.emplace_back([&](const Environment &Env) {
> return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> });
> }
> ```
> I'm not sure if it's worth the trouble, but if I really had to bother, I
> would do something like the above.
That wouldn't work, since the pass is only executed afterwards.
================
Comment at: clang/lib/Format/Format.cpp:3486-3489
+ Expanded.InsertBraces = true;
+ Passes.emplace_back([&](const Environment &Env) {
+ return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
});
----------------
HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > owenpan wrote:
> > > tahonermann wrote:
> > > > How about using an init capture instead? This will suffice to avoid one
> > > > of the copies but means that `InsertBraces` doesn't get set until the
> > > > lambda is invoked. I wouldn't expect that to matter though.
> > > I'm not sure if it's worth the trouble, but if I really had to bother, I
> > > would do something like the above.
> > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't
> > this bascially do the same without us having to remember to put Expanded
> > back afterwards? I don't see how using Expanded is any different from using
> > S (other than the copy)
> >
> > ```
> > if (Style.InsertBraces) {
> > FormatStyle S = Expanded;
> > S.InsertBraces = true;
> > Passes.emplace_back([&](const Environment &Env) {
> > return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > });
> > }
> > ```
> > I'm not sure if it's worth the trouble, but if I really had to bother, I
> > would do something like the above.
>
> That wouldn't work, since the pass is only executed afterwards.
> Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't this
> bascially do the same without us having to remember to put Expanded back
> afterwards? I don't see how using Expanded is any different from using S
> (other than the copy)
>
> ```
> if (Style.InsertBraces) {
> FormatStyle S = Expanded;
> S.InsertBraces = true;
> Passes.emplace_back([&](const Environment &Env) {
> return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> });
> }
> ```
That would have a dangling reference to S and most likely don't work as wished.
================
Comment at: clang/lib/Format/Format.cpp:3486-3489
+ Expanded.InsertBraces = true;
+ Passes.emplace_back([&](const Environment &Env) {
+ return BracesInserter(Env, Expanded).process(/*SkipAnnotation=*/true);
});
----------------
HazardyKnusperkeks wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > owenpan wrote:
> > > > tahonermann wrote:
> > > > > How about using an init capture instead? This will suffice to avoid
> > > > > one of the copies but means that `InsertBraces` doesn't get set until
> > > > > the lambda is invoked. I wouldn't expect that to matter though.
> > > > I'm not sure if it's worth the trouble, but if I really had to bother,
> > > > I would do something like the above.
> > > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't
> > > this bascially do the same without us having to remember to put Expanded
> > > back afterwards? I don't see how using Expanded is any different from
> > > using S (other than the copy)
> > >
> > > ```
> > > if (Style.InsertBraces) {
> > > FormatStyle S = Expanded;
> > > S.InsertBraces = true;
> > > Passes.emplace_back([&](const Environment &Env) {
> > > return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > > });
> > > }
> > > ```
> > > I'm not sure if it's worth the trouble, but if I really had to bother, I
> > > would do something like the above.
> >
> > That wouldn't work, since the pass is only executed afterwards.
> > Apart from perhaps the unnecessary copying from Expanded -> S.. doesn't
> > this bascially do the same without us having to remember to put Expanded
> > back afterwards? I don't see how using Expanded is any different from using
> > S (other than the copy)
> >
> > ```
> > if (Style.InsertBraces) {
> > FormatStyle S = Expanded;
> > S.InsertBraces = true;
> > Passes.emplace_back([&](const Environment &Env) {
> > return BracesInserter(Env, S).process(/*SkipAnnotation=*/true);
> > });
> > }
> > ```
>
> That would have a dangling reference to S and most likely don't work as
> wished.
This should work. One could replace the two assignments with a RAII wrapper
which restores the old value.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149647/new/
https://reviews.llvm.org/D149647
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
