MyDeveloperDay added inline comments.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3524
return false;
if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator))
return false;
----------------
mprobst wrote:
> curdeius wrote:
> > HazardyKnusperkeks wrote:
> > > mprobst wrote:
> > > > MyDeveloperDay wrote:
> > > > > mprobst wrote:
> > > > > > shouldn't you change this line here?
> > > > > You know I thought the same and this was where I first put the code
> > > > > change in, but this code doesn't seem to have any effect and I've
> > > > > been caught out by this before... (anyone else seen that?)
> > > > >
> > > > > I'm not sure if something has been changed, but I'm finding that
> > > > > often I add code into spaceRequiredBetween() and despite it being
> > > > > executed correctly it doesn't have the desired effect, which is why
> > > > > the code is in spaceRequiredBefore()
> > > > Generally we put space between two operators. So this line must have
> > > > some effect, as otherwise we'd always emit `A | B`. Given that, I think
> > > > we need more debugging here to make this change - working around by
> > > > some more code somewhere else doesn't seem like a good long term
> > > > approach.
> > > >
> > > > Also, note that this is all in `spaceRequiredBefore`, right?
> > > It's on my plan to refactor these 2 methods, because I think they can and
> > > should be made easier to understand and reason about.
> > >
> > > So from my point of view one can accept this patch.
> > I modified it like this and it passes all the tests. Please recheck.
> I appreciate the intention to refactor this in the future.
>
> I still believe we really have to understand how this code works though, and
> why changing this line doesn't have the effect we'd expect, before we land
> the fix. Chances are there is some substantial misunderstanding how this all
> fits together, which usually leads to bugs.
@mprobst I do agree, let me take another look.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117197/new/
https://reviews.llvm.org/D117197
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
