[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

In D140996#4185795 , @bolshakov-a 
wrote:

> @royjacobson, I've added some test cases for using the new NTTP arguments in 
> clang modules. It uses serialization, in principle. Or more specialized tests 
> are still needed?

No, I think that's good. Thanks for adding them!


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

https://reviews.llvm.org/D140996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

There are no AST [de]serialization tests in this PR, right? Would be nice to 
add some.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

In D130689#3709834 , @thieta wrote:

> In D130689#3709742 , @aaron.ballman 
> wrote:
>
>> One thing I think would be a definite improvement is to have done an RFC on 
>> Discourse for these changes so that downstreams have a chance to weigh in on 
>> the impact. The patch was put up on Jul 28 and landed about a week later 
>> without any notification to the rest of the community who might not be 
>> watching cfe-commits -- that's a very fast turnaround and very little 
>> notification for such a significant change.
>
> Yeah this is on me. Honestly I didn't expect it to be that much of a problem 
> but rather the toolchain requirement we posted as part of it would be the big 
> hurdle where bot owners would have to upgrade to get the right versions. But 
> lesson learned  and we should add some more delays in the policy here: 
> https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the C++ standards 
> upgrade.

Two points I want to add that I think would've been useful as well -

1. In addition to the toolchain soft errors, add a version check + #warning to 
some llvm header. This would be useful as it is more visible than the CMake 
warning and it could show up for cases where LLVM is used as a library+headers 
and not built from sources.
2. Delay actual usage of new language features until after the next release. 
Currently I see people pushing lots of cleanup commits that could hurt bug 
backports. It also has the benefit of making the transition more gradual.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

In D130689#3705145 , @thieta wrote:

> In D130689#3705131 , @royjacobson 
> wrote:
>
>> This seems to have been more disruptive than expected, since an existing 
>> CMakeCache.txt can make LLVM compile in previous C++14 configuration. This 
>> seems to make some of the bots fail in a way that makes the patches making 
>> use of C++17 features seem at fault.
>>
>> See:
>> https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
>> https://reviews.llvm.org/rG32fd0b7fd5ab
>>
>> How would you feel about adding something like
>>
>>   #if defined(__cplusplus) && __cplusplus < 201703L
>>   #error "LLVM requires at least C++17"
>>   #endif
>>
>> to some central header, to make this switch more visible?
>
> I am not opposed to that directly. But this seems a bit dangerous where bots 
> retain the cmakecache - there must be other cases where we can't really 
> protect in this way.
>
> Another approach would be to unset CMAKE_CXX_STANDARD if it's below 17 in 
> cmake directly.
>
> But in general - I am not a huge fan of CI / bots trying to keep the cache 
> around - many weird issues can arise from this.

This affects people on their work branches as well, and it's not obvious that 
it's a configuration error and not a broken master.

The CMake approach sounds cleaner to me, but I don't know CMake well enough to 
do it - if you could post a follow up patch I think it would be quite helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

This seems to have been more disruptive than expected, since an existing 
CMakeCache.txt can make LLVM compile in previous C++14 configuration. This 
seems to make some of the bots fail in a way that makes the patches making use 
of C++17 features seem at fault.

See:
https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
https://reviews.llvm.org/rG32fd0b7fd5ab

How would you feel about adding something like

  #if defined(__cplusplus) && __cplusplus < 201703L
  #error "LLVM requires at least C++17"
  #endif

to some central header, to make this switch more visible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits