[PATCH] D117197: [clang-format] Add new option to support adding a space between Javascript Union and Intersection types

[Martin Probst via Phabricator via cfe-commits]

mprobst added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3524
       return false;
     if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator))
       return false;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117197/new/

https://reviews.llvm.org/D117197