curdeius requested changes to this revision. curdeius added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/lib/Format/TokenAnnotator.cpp:3524-3525 return false; if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator)) return false; if ((Left.is(tok::l_brace) || Right.is(tok::r_brace)) && ---------------- 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. 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