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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3475
       return false;
+    if (Left.is(TT_JsTypeOperator) && Right.isTypeOrIdentifier() ||
+        (Left.isTypeOrIdentifier() || Left.is(TT_TemplateCloser)) &&
----------------
Be explicit on the binding.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117197