- User Since
- Jul 29 2013, 1:59 AM (373 w, 5 d)
Finally I've opted for creating a small revision just fixing this particular bug report.
Mind however that the problem is a bit more complex and to solve all possible cases we would need to first reformat the source code, then sort the includes and then again, possibly reformat parts of the source code modified by sorting, so that the comments get re-aligned etc.
- Ooops. Revert unwanted test changes.
Wed, Sep 23
if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret)) return true; // << HERE
It should return false if everything fits on a single line.
I'm not sure how to check this though.
@MyDeveloperDay, would you have an idea?
Thu, Sep 17
Don't we risk any other side effects?
Aug 11 2020
LGTM. Thank you for the patch!
Aug 7 2020
Please take a look at the discussion in https://reviews.llvm.org/D80376 that has been put on standby because the desired ABI needs compiler support.
Jul 30 2020
Jul 22 2020
Jul 18 2020
The changes look good to me in general. I share your doubt though about whether a bool flag is sufficient here. We've seen in the past a few times that at some time a false/true flag is not enough. I'd rather go for a Before/After/Both/None flag (or similar, naming probably should be coherent with other flags). But I'm not really aware of the projects/coding styles that use bit fields. Maybe a small research on this would be good to confirm or infirm a necessity of higher complexity.
Jul 12 2020
LGTM! Thanks for fixing this.
Jul 2 2020
Jun 26 2020
LGTM. Thank you for taking my comments into account.
Jun 23 2020
As a general remark, it would be call if you added a link to standard draft or proposal (or both) in the summary of each revision. It would make reviewing a bit easier :).
My 2 cents.
Jun 19 2020
I've updated the "unsupported" comments with precise compiler versions. I cannot test apple-clang though.
Anyway, I'll put this revision into hibernation until there's a builtin for a single-pointer source location (whether it be __bultin_source_location or T* __builtin_static_storage_duration(T)).
- Use precise compiler versions in UNSUPPORTED.
Jun 18 2020
Ok, so clang 8 doesn't support consteval. It also doesn't have required builtins (__builtin_FILE etc.). consteval works ok on clang 9+.
How can I mark tests as unsupported when the builtins are not supported? (a good example would be column.pass.cpp test that I disabled for gcc that doesn't have __builtin_COLUMN)
- Address review comments.
Jun 17 2020
Jun 3 2020
For a second, I thought that you could simplify the code by removing this @try condition (and calling the function FormatTokenLexer::tryTransformTryUsageForC() only if isCppOnly, but given that Objective is a superset of C, it's probably safer to keep it the way it's done now.
Keep in mind I'm not the code owner, I don't know if another approval is required.
Nice. Should we test other non-C keywords as well?
Out of curiosity, where does "@try" come from?
I would have nothing against if you'd added this option and we kept current behaviour by default.
The only drawback is the (bit of) complexity it adds bit that seems justified to me.
Jun 2 2020
The change seems to me technically sound, but I'm not sure of the scope of its effects. There might be users that rely on this behaviour. On the other hand, adding an option to keep the old behaviour doesn't seem appropriate, and personally I consider the old behaviour as a bug.
May 30 2020
OK, great. Thanks for all the information.
LGTM. But you need to wait for a review from somebody from libc++ group.
May 29 2020
It may be silly, but the fact that this code runs over all the tokens makes me think: do we have any performance-related non-regression tests for clang-format?
That looks nicer indeed. Thanks.
Just some minor nitty-gritty comments.
@MyDeveloperDay, I've played around with the script, you can take as much as you judge useful from here: https://github.com/mkurdej/llvm-project/tree/arcpatch-D80627.
@MyDeveloperDay , I know it's a strange request, but could you change (or remove) the background color for 100% case.
I'm partially color-blind and having red and green background in the same table is really hard to distinguish. I guess I'm not alone.
I'd suggest using something like light blue, it doesn't need to stand out anyway.
May 28 2020
First of all, very nice idea. :)
Second, could you please make sure that the script is compatible with Python 3?
Also, in order to clean up a bit the generation of the RST (to avoid the repetition of all this output.write() stuff), you can use multiline strings """, or in general, take inspiration from e.g. https://github.com/llvm/llvm-project/blob/master/libcxx/utils/generate_feature_test_macro_components.py#L720.
Lastly, it would be nice to add last update date somewhere.
May 27 2020
One last thought, how about making the config a bit more future-proof and instead of ConstPlacement accept what was discussed some time ago:
QualifierStyle: - const: Right
and restrict it to const for the moment?
Maybe renaming to QualifierPlacement or something more appropriate.
May 26 2020
May 25 2020
Ok. I'll create another revision with necessary .clang-format (and related) changes.
May 23 2020
Ok, fair enough. I'll abandon this revision then.
Should we then update .clang-format (at least for the directory where auto-generated tests are) or update the python script so that it matches libc++ style?
For the former, IndentPPDirectives should be set to AfterHash.
May 22 2020
I have a small doubt. Should <cmath> include lerp in its synopsis? It's currently missing. I can add it in this patch if needed.
I've just realized that there is already D77505. You should probably think about helping to incorporate your changes into this other PR and abandon this one.
Thanks for the review.
May 21 2020
I like these changes.
I have mixed feelings about isCpp() & co.
As @MyDeveloperDay said, I'd like it mean C++ only. I find it confusing that it means C++ or ObjC (even if the latter is a superset of the former). I'd rather see it spelt as isCppOrObjC() even if it's verbose but at least removes all confusion IMO.
- Add new header to CMake, double-include test and module map.
- Revert "Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function."
- Reorder include.
- Rebase on master.
May 20 2020
May 15 2020
I've updated the status page in commit 182adf120ccffe937d95d5b447f6506f82f615ec. I guess it was an oversight.
- Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function.
Well, I redefined _NOEXCEPT to nothing, but there are hacks involved.
First of all, libc++ headers include <__config> that defines _NOEXCEPT unconditionally. So I needed to make it conditional.
Secondly, to make this work I replaced noexcept with _NOEXCEPT on day() method in <chrono>.
And yeah, then I defined _NOEXCEPT to nothing, but it seems to me a dangerous endeavour.
May 13 2020
FYI, there are about 130 occurrences of #define _LIBCPP_ASSERT in tests, but debug_less.pass.cpp is the only place where throw is used (in other cases, exit is called).
Do you know what was the purpose of redefining it?
Reopen this revision as there was a failure in test/libcxx/algorithms/debug_less.pass.cpp that defined _LIBCPP_ASSERT macro to throw.
The proposed fix includes <chrono> before redefining this macro.
Reopening revision due to failures in test/libcxx/algorithms/debug_less.pass.cpp.
May 7 2020
@ldionne, I'd appreciate if you (or somebody else) could tell me what you think about the way I test the constexpr. I wanted to avoid duplication but on a second thought, I think that run-time tests can actually go through compile-time (constexpr) branch and so leave the non-constexpr branch untested.
Another thing in which I'd need guidance is how to test the primary implementation (not specializations for float, double, long double). Is there another floating-point type I can use in tests? Maybe some sort of short float?
Oh, and should integral types be tested with complex<T>?