Nuullll added inline comments.
================ Comment at: clang/lib/Format/TokenAnnotator.cpp:236 + Prev = Prev->Previous; + assert(Prev); + } ---------------- MyDeveloperDay wrote: > curdeius wrote: > > MyDeveloperDay wrote: > > > Do we need to worry about `Prev` ever being null? Does the assert ever > > > fire? if not why have it in the first place? > > > > > > We'll crash if it is, do we want to guard against that? > > Disclaimer: I'm in the pro-assert camp :). > > In general, I'd preferably keep this assert because people sometimes use > > Release+Assertions build. Even more those that report bugs. > > And it's much nicer and easier to fix an assertion failure that is well > > defined and precisely localised than a less clearer report of a crash. > > Of course, in a build without assertions, it will crash if `Prev` is null, > > but still it will be easier to find the bug when trying to reproduce. > > > > For this specific case, this assertion should never fire because otherwise > > it would mean that we detected had `Left` being > > `TT_OverloadedOperatorLParen` but there was no corresponding > > `tok::kw_operator` (or just `operator`). So it would mean that there's a > > bug in (probably) token annotator. So yeah, it falls into "should never > > happen" category about which assertions are all about IMO. > To be honest I'm in the assert and guard as well camp (if such a camp exists > ;-) > > I have this discussion regularly with the other developers in my team, the > use assert is useless other than for our assertion builds (which in high > performance code, almost no one runs, until there is a problem!) > > The assert triggers in my mind that it MIGHT be null (but generally > shouldn't) so in which case we have to guard as well for the rare case, but I > like the assert.. but please of the form: > > ```assert(condition && "Some nice string telling me exactly what went > wrong")``` > > When the assert fires I get a nice error message, not just assert failed with > `0` > > https://llvm.org/docs/CodingStandards.html#assert-liberally > @curdeius @MyDeveloperDay Thanks for the explanations. I've added an error message. ================ Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286 + // TODO: + // Calling an operator as a non-member function is hard to distinguish + // verifyFormat("void f() { operator*(a & a); }"); + // verifyFormat("void f() { operator&(a, b * b); }"); ---------------- MyDeveloperDay wrote: > curdeius wrote: > > curdeius wrote: > > > Nuullll wrote: > > > > This patch doesn't fix these, i.e. when operators are called as > > > > non-member functions. > > > > The call sites seem to be marked as function declarations. > > > Is there a bug report on that? If so, please add a link to the comment. > > > If no, could you please create one? > > > Or is it really something that will never be possible to distinguish > > > (hmm, this would be strange)? > > Nit: full-stop. > Might be nice just to fix these as well, this is kind of what happens with > clang-format development... you fix one thing we try to make you fix > everything else around it, otherwise who is going to understand it better > than you! ;-) > > We do appreciate your patch!! @curdeius > Is there a bug report on that? If so, please add a link to the comment. If > no, could you please create one? No. I've created one and added the link in the comment! > Or is it really something that will never be possible to distinguish (hmm, > this would be strange)? I think it should be distinguishable, at least there should be no return type before the operator in the context of non-member function invocation. I'd like to fix this in a separate patch, later. I need some time to figure it out :) ================ Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286 + // TODO: + // Calling an operator as a non-member function is hard to distinguish + // verifyFormat("void f() { operator*(a & a); }"); + // verifyFormat("void f() { operator&(a, b * b); }"); ---------------- Nuullll wrote: > MyDeveloperDay wrote: > > curdeius wrote: > > > curdeius wrote: > > > > Nuullll wrote: > > > > > This patch doesn't fix these, i.e. when operators are called as > > > > > non-member functions. > > > > > The call sites seem to be marked as function declarations. > > > > Is there a bug report on that? If so, please add a link to the comment. > > > > If no, could you please create one? > > > > Or is it really something that will never be possible to distinguish > > > > (hmm, this would be strange)? > > > Nit: full-stop. > > Might be nice just to fix these as well, this is kind of what happens with > > clang-format development... you fix one thing we try to make you fix > > everything else around it, otherwise who is going to understand it better > > than you! ;-) > > > > We do appreciate your patch!! > @curdeius > > > Is there a bug report on that? If so, please add a link to the comment. If > > no, could you please create one? > > No. I've created one and added the link in the comment! > > > Or is it really something that will never be possible to distinguish (hmm, > > this would be strange)? > > I think it should be distinguishable, at least there should be no return type > before the operator in the context of non-member function invocation. I'd > like to fix this in a separate patch, later. I need some time to figure it > out :) > Might be nice just to fix these as well, this is kind of what happens with > clang-format development... you fix one thing we try to make you fix > everything else around it, otherwise who is going to understand it better > than you! ;-) > > We do appreciate your patch!! @MyDeveloperDay Thanks for reviewing this. I will try to fix these in another patch. It's always exciting to contribute to llvm! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103678/new/ https://reviews.llvm.org/D103678 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
