[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Any update on this patchset? With the migration away from phabricator, it'd be 
good to get this in soonish (otherwise it'll need to be moved to github).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869

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

[PATCH] D135341: adds `__reference_constructs_from_temporary`

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/www/cxx_status.html:297
+  trait builtin, with which 
std::reference_constructs_from_temporary
+  implemented. __reference_converts_from_temporary needs to be
   provided, following the normal cross-vendor convention to implement




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135341

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

[PATCH] D129635: [OpenMP] Update the default version of OpenMP to 5.1

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D129635#4451189 , @saiislam wrote:>

> Are there any features in this table which have been already implemented but 
> have not been tagged?
> https://clang.llvm.org/docs/OpenMPSupport.html#openmp-5-1-implementation-details

I ended up opening an issue for this: 
https://github.com/llvm/llvm-project/issues/63633


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129635

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

[PATCH] D129635: [OpenMP] Update the default version of OpenMP to 5.1

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D129635#4440613 , @animeshk-amd 
wrote:

> In the multi-company community meeting, the agreement was to move to the 5.1 
> version assuming that these features are supported.

We shouldn't need to assume - either the features are supported or not. I 
thought the status page would be the right place for this information, but 
perhaps it is out of date? Whoever the openmp stakeholders are here should 
ensure this information is correct and up-to-date!

I mean, I'm sure the participants in that meeting know the situation much 
better than I do, but from what's visible from the outside, it looks unusual to 
default to something that's not yet fully implemented (for all the usual 
reasons: assuming there are mistakes found in the not-yet-complete 
implementation of 5.1, you'll then have to break your users to fix it, whereas 
until this PR, it was an explicit choice of the consumer to use the 
not-yet-fully-supported 5.1; it's also unusual in the way that a user will get 
an error for using features that aren't implemented yet, despite being able to 
see that the default is 5.1)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129635

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

[PATCH] D153379: Remove -flang-experimental-exec flag

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Does this merit a mention in the release notes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153379

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

[PATCH] D129635: [OpenMP] Update the default version of OpenMP to 5.1

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Does https://clang.llvm.org/docs/OpenMPSupport.html need an update? It still 
says "Clang fully supports OpenMP 4.5" (with many 5.0/5.1 features marked as 
"worked on" / "unclaimed"), which would make it unusual to put the default on a 
version that's (according to that status page) only ~30% implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129635

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

[PATCH] D152946: [C++20][Modules] Implement P2615R1 revised export diagnostics.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Mark P2615  as implemented in 
https://github.com/llvm/llvm-project/blame/main/clang/www/cxx_status.html?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152946

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

[PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: libunwind/src/CMakeLists.txt:28-35
 
 # See add_asm_sources() in compiler-rt for explanation of this workaround.
 # CMake doesn't work correctly with assembly on AIX. Workaround by compiling
 # as C files as well.
 if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR
-   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17) OR
-   (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
+   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
   set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)

mstorsjo wrote:
> h-vetinari wrote:
> > Shouldn't it be possible to remove that entire block (as it only fires for 
> > CMake <3.20)?
> The intention was to do such cleanups in a later patch, as mentioned in the 
> original commit message in https://reviews.llvm.org/D144509. (I guess it 
> would be good to bring the original commit message along with the reland 
> attempts too.)
Well sure, but "workarounds" is a broad term. It makes sense to not try to 
clean up everything that newer CMake enables, but this PR does remove a lot of 
(all?) other lines with `CMAKE_VERSION VERSION_LESS ` where x<3.20. On top 
of that, since this line is already being changed (which is why I noticed in 
the first place), I'd suggest to just remove it. But I don't feel strongly 
about this, just thought I'd point it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344

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

[PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: libunwind/src/CMakeLists.txt:28-35
 
 # See add_asm_sources() in compiler-rt for explanation of this workaround.
 # CMake doesn't work correctly with assembly on AIX. Workaround by compiling
 # as C files as well.
 if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR
-   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17) OR
-   (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
+   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
   set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)

Shouldn't it be possible to remove that entire block (as it only fires for 
CMake <3.20)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344

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

[PATCH] D150450: Add C++26 compile flags.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/include/clang/Basic/LangStandard.h:128
+
+  // hasDigraphs - Language supports digraphs.
   bool hasDigraphs() const { return Flags & Digraphs; }

Spuriously removed slash



Comment at: clang/www/cxx_status.html:1568
 
+C++2c implementation status
+

The longer this page gets, the more I feel it should be sorted in reverse 
chronological order of the standard versions. It's bothersome IMO to keep 
scrolling past almost-fully implemented standards (currently the tables for 
C++17 & C++20) before getting to see the status of the latest & greatest. 

https://en.cppreference.com/w/cpp/compiler_support is sorted in reverse order 
as well.


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

https://reviews.llvm.org/D150450

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

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/www/cxx_status.html:1485-1486
+Clang 17 (Partial)
+ We do not support outside of defaulted special memeber functions the 
change that constexpr functions no
+  longer have to be constexpr compatible but rather support a less 
restricted requirements for constexpr
+  functions. Which include allowing non-literal types as return values 
and paremeters, allow calling of

That sentence is impossible to parse for me. Is "outside of defaulted special 
member functions" supposed to be a subclause? The second sentence is also not 
great ("Which include [X], allow [Y]").

Also typos: "memeber" and "paremeters".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146090

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

[PATCH] D144157: Add 128-bit integer support to enum element

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:19571-19573
-  // TODO: If the result value doesn't fit in an int, it must be a long or long
-  // long value.  ISO C does not support this, but GCC does as an extension,
-  // emit a warning.

That comment is still relevant AFAICT, at least partially (the ISO C 
restrictions are still there, GCC still extends it)


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

https://reviews.llvm.org/D144157

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

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/4)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

> Sorry for the misleading and thanks for the quick clarification. So it looks 
> like the status quo is a little bit worse than I imaged...

I opened a thread on discourse for more visibility: 
https://discourse.llvm.org/t/land-c-modules-changes-required-for-cmake-before-llvm-16-branch/67717


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

https://reviews.llvm.org/D137534

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

[PATCH] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/4)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D137534#4055283 , @ChuanqiXu wrote:

> @h-vetinari said it should be OK to backport these patches to branched 16.x.

That should be considered a comment from the peanut gallery, not a definite 
statement. What I had said in https://reviews.llvm.org/D137058 was:

>> [...] but I am not sure if we can land them in time. I guess it may be 
>> possible to backport these patches to 16.x since it is relatively important?
>
> Might make sense tagging the release managers if things get tight - perhaps 
> it's possible. E.g. ranges for LLVM 15 was also finished after the branch.




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

https://reviews.llvm.org/D137534

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:5639-5640
+
+  // If we're emitting a module output with the speicifed option
+  // `-fmodule-output`.
+  if (!AtTopLevel && isa(JA) &&





Comment at: clang/lib/Driver/Driver.cpp:5736
+  C.getArgs().hasArg(options::OPT_fmodule_output) &&
+  C.getArgs().hasArg(options::OPT_o)) {
+SmallString<128> OutputPath;

dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > tahonermann wrote:
> > > > dblaikie wrote:
> > > > > ChuanqiXu wrote:
> > > > > > dblaikie wrote:
> > > > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) 
> > > > > > > being a special case? It'd be good if we could use a single 
> > > > > > > common implementation regardless of whether `-o` is used (ie: 
> > > > > > > Figure out the output file name (whether it's implicitly 
> > > > > > > determined by clang, in the absence of `-o`, or explicitly 
> > > > > > > provided by the user through `-o`) and then replace the suffix 
> > > > > > > with `pcm`)?
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > > file will always lives in the same directory with the input file.
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm 
> > > > > > file will always lives in the same directory with the input file.
> > > > > 
> > > > > I don't follow/understand. What I mean is, I'd like it, if possible, 
> > > > > this was implemented by something like "find the path for the .o file 
> > > > > output, then replace the extension with .pcm".
> > > > > 
> > > > > I worry atht code like this that recomputes something similar to the 
> > > > > object output path risks getting out of sync with the actual object 
> > > > > path.
> > > > That is certainly a valid concern and I agree it would be better if the 
> > > > shared output path is computed exactly once. If that would require 
> > > > significant change, then tests to ensure consistent behavior would be 
> > > > the next best thing. I'm not sure what infrastructure is in place for 
> > > > validation of output file locations though.
> > > Well, I guess the Split DWARF file naming isn't fundamentally better - it 
> > > looks at the OPT_O argument directly too:
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > > 
> > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > function in some way? (for instance, it checks `-c` too, which seems 
> > > relevant - we wouldn't want to name the .pcm after the linked executable 
> > > name, right?
> > > 
> > > Perhaps we could at least reuse this same logic - make it a reusable 
> > > function in some way? 
> > 
> > Done. It looks better now.
> > 
> > > (for instance, it checks -c too, which seems relevant - we wouldn't want 
> > > to name the .pcm after the linked executable name, right?
> > 
> > Oh, right. Although the previous conclusion is that if `-o` is specified, 
> > the .pcm file should be in the same dir with the output. But it is indeed 
> > weird that the .pcm file are related to the linked executable if we thought 
> > it deeper. 
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
> chance it could be refactored into a common implementation between Split 
> DWARF and modules?
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any 
> chance it could be refactored into a common implementation between Split 
> DWARF and modules?

Could we uncouple this clean-up from landing the patches before the LLVM 16 
branch? A trivial refactor might even still be backportable, but the whole 
series will be more challenging.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

> BTW, the series of clang-scan-deps patch (https://reviews.llvm.org/D139168) 
> is also necessary [...]

Yes, I meant both those series.

> [...] but I am not sure if we can land them in time. I guess it may be 
> possible to backport these patches to 16.x since it is relatively important?

Might make sense tagging the release managers if things get tight - perhaps 
it's possible. E.g. ranges for LLVM 15 was also finished after the branch.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Without undue haste, I just wanted to comment from the peanut gallery that it 
would be amazing if the patches that are necessary for CMake + Clang to use C++ 
modules would make the cut-off for LLVM 16 that's coming up around the 24th of 
January.


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

https://reviews.llvm.org/D137058

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

[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/test/Driver/save-std-c++-module-file.cpp:1
+// RUN: rm -rf %t
+// RUN: mkdir %t

The filename of this test still reflects the old option name and should 
presumably be changed.


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

https://reviews.llvm.org/D137058

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

[PATCH] D139212: [Clang] make_cxx_dr_status download the issue list automatically

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

I've had a small nit/suggestion post-merge of https://reviews.llvm.org/D138901; 
in case you feel it's worth picking up if you're touching that file already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139212

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

[PATCH] D138901: [clang] Propely handle tests for open DRs in make_cxx_dr_status

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

I'd also use newlines more liberally since the two-space indentation is pretty 
dense already.




Comment at: clang/www/make_cxx_dr_status:190-191
+ % (dr.issue, unresolved_status, dr.status)
+if not avail.startswith('Sup') and not avail.startswith('Dup'):
+  count[avail] = count.get(avail, 0) + 1
   else:

These two lines are the same with the end of the else-branch, i.e. they could 
both be lifted out of the if-clause and unified at the end.



Comment at: clang/www/make_cxx_dr_status:198-199
+   % (dr.issue, unresolved_status)
 if not avail.startswith('Sup') and not avail.startswith('Dup'):
   count[avail] = count.get(avail, 0) + 1
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138901

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

[PATCH] D134128: [P0857R0 Part B] Resubmit an implemention for constrained template template parameters

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Congratulations for landing this!

How far do you (both) think we're away from completing concepts? Are the issues 
(aside from CWG1496 & CWG1734) mentioned on 
https://clang.llvm.org/cxx_status.html tracked anywhere else? I see that 
https://github.com/orgs/llvm/projects/14 has 45 concept-related issues, but 
presumably not all of those are necessary to set the feature macro?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

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

[PATCH] D134128: [P0857R0 Part B] Resubmit an implemention for constrained template template parameters

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:554
+- Implemented `P0857R0 
`_,
+  which words for constrained lambdas and constrained template 
*template-parameter*\s.
 

erichkeane wrote:
> erichkeane wrote:
> > I'm not sure what you're trying to say here?  Is there just a typo that I'm 
> > missing that makes this perfectly clear? 
> Sorry, I'm still getting caught up on what 'which words' means here?  
Maybe the intention was something along the lines of: `words` -> `specifies` 



Comment at: clang/test/SemaTemplate/concepts.cpp:62
 
   // FIXME: This is valid under P0857R0.
   template concept C = true;

That `FIXME` should be removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128

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

[PATCH] D133853: [AST] Add msvc-specific C++11 attributes

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

> Now I'm wondering why the attribute exists at all. If it's functionally 
> equivalent to `constexpr` as a keyword, what are the use cases for the 
> attribute?

I'm guessing something to do with ABI-compatibility with artefacts produced by 
their older compilers (though I have no idea what scenario they have come up 
with that would break). It's also for a very recent version (17.3, the last 
non-preview release as of writing), and I cannot find documentation for it.

Though while searching for it, I stumbled over 
https://reviews.llvm.org/D110485, which relates to `[[no_unique_address]]` & 
how it's incompatible to use it for clang on windows if MSVC doesn't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133853

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

[PATCH] D133853: [AST] Add msvc-specific C++11 attributes

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D133853#3797598 , @RIscRIpt wrote:

> I am afraid it would take me some effort to implement semantics of 
> `[[msvc::no_unique_address]]`, so I'd like to focus only on 
> `[[msvc::constexpr]]` in current patch.

Just for context, `[[msvc::no_unique_address]]` is 100% 

 like the standardized `[[no_unique_address]]`, but MSFT was... exceedingly 
cautious... about introducing this, as - even for an explicitly opt-in new 
feature - someone might have compiled code containing `[[no_unique_address]]` 
at a time when it was a no-op in their compiler (as an unknown attribute), and 
changing the semantics after the fact would have violated their very strict 
compatibility guarantees.

I assume the reasoning behind `[[msvc::constexpr]]` to be along the same lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133853

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

[PATCH] D131388: [docs] Add "Standard C++ Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D131388#3748062 , @ChuanqiXu wrote:

> Replace `C++20 Modules` with `Standard C++ Modules` since @ruoso pointed out 
> that `Modules` is not specific to certain versions of C++ (for example, 
> Modules may get some big changes in C++23, C++26, ...).

The filename (and hence future doc URL) still contains the "20".


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

https://reviews.llvm.org/D131388

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

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

I have another question, probably mainly for @tstellar (since I don't 
understand the `clang/tools/libclang/libclang.{exports,map}` infrastructure). 
Now that we're defaulting back to the equality case, would we need to reinstate 
`libclang.exports`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132486

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

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/tools/libclang/CMakeLists.txt:9-12
+else()
+  # ... unless explicily overridden
+  set(LIBCLANG_SOVERSION ${CLANG_VERSION_MAJOR})
+endif()

Sorry I didn't get to comment in time, but now that the default was switched 
from OFF to ON, the comments here are lying...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132486

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

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

> My concerns have already been raised by others in that thread and related 
> issues, I see no point in restating them yet again. I don't see consensus, I 
> see a handful of people discussing reverting a change that broke a whole 
> bunch of assumptions made by real-world code.

I don't think that's a fair characterization. The release manager (as well as 
author of bot the original change & the reversion) and the interim release 
manager discussed this and came to the same conclusion despite being very aware 
of the complaints. Other than that there was one comment against where the 
person did not respond to a request for further information & 3 people who were 
asking to keep the ABI information.




Comment at: clang/CMakeLists.txt:467
+option(CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION
+  "Force the SOVERSION of libclang to be equal to CLANG_MAJOR" OFF)
+

jrtc27 wrote:
> h-vetinari wrote:
> > jrtc27 wrote:
> > > OFF by default changes behaviour, which seems irresponsible so late in 
> > > the release cycle
> > I disagree. This is the status of the last released version.
> It's the status of the last release candidate, something which downstreams 
> are encouraged to consume early in order to give feedback. If we only need to 
> care about the last released version when making changes then what's the 
> point of having a release branch with policies around what's ok to backport?
This kind of change is a good example why there are release candidates. We 
raised this shortly after rc2 (I built them as fast as possible, given that 
limits on public CI means this is a multistep process that can take a bit).



Comment at: clang/tools/libclang/CMakeLists.txt:14-17
+# TODO: harmonize usage of LIBCLANG_SOVERSION / LIBCLANG_LIBARY_VERSION
+#   below; this was added under time-pressure to avoid reverting the
+#   better default from LLVM 14 for LLVM 15.0.0-rc3, hence no time
+#   to clean up previous inconsistencies.

jrtc27 wrote:
> h-vetinari wrote:
> > jrtc27 wrote:
> > > This is highly subjective. Many believe the default was worse due to (a) 
> > > confusion (b) technical issues when coinstalling multiple LLVM versions. 
> > > Ping-ponging like this is just creating a mess.
> > The ping pong has not been released yet, and so it's "just" happening as 
> > part of the development process so far.
> > 
> > It's subjective how this changes ranks vis-à-vis someone's personal 
> > preferences/priorities, but I'd argue it's objectively better from a 
> > technical perspective, as it keeps strictly more information as otherwise. 
> > On top of that we're solving (b) for those who do not want to make use of 
> > this additional information, or are bothered by it.
> Except people package the rc's and get annoyed when major changes like this 
> are made; if we ask for people to test things, then go and revert something 
> significant at the last minute, that's not good practice.
Which is why we're trying to get this into the next/last rc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132486

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

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Thanks for the review. Given that you have concerns, could you voice them in a 
larger forum 
(https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410),
 where so far the direction was in favour of going back to the status of LLVM 
14 (but with an opt-out for those who prefer equality).




Comment at: clang/CMakeLists.txt:467
+option(CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION
+  "Force the SOVERSION of libclang to be equal to CLANG_MAJOR" OFF)
+

jrtc27 wrote:
> OFF by default changes behaviour, which seems irresponsible so late in the 
> release cycle
I disagree. This is the status of the last released version.



Comment at: clang/tools/libclang/CMakeLists.txt:6
+
+if(NOT CLANG_FORCE_MATCHING_LIBCLANG_VERSION)
+  # default is to use the SOVERSION according to ABI...

jrtc27 wrote:
> Here says VERSION, clang/CMakeLists.txt says SOVERSION
Thanks for catching this!



Comment at: clang/tools/libclang/CMakeLists.txt:14-17
+# TODO: harmonize usage of LIBCLANG_SOVERSION / LIBCLANG_LIBARY_VERSION
+#   below; this was added under time-pressure to avoid reverting the
+#   better default from LLVM 14 for LLVM 15.0.0-rc3, hence no time
+#   to clean up previous inconsistencies.

jrtc27 wrote:
> This is highly subjective. Many believe the default was worse due to (a) 
> confusion (b) technical issues when coinstalling multiple LLVM versions. 
> Ping-ponging like this is just creating a mess.
The ping pong has not been released yet, and so it's "just" happening as part 
of the development process so far.

It's subjective how this changes ranks vis-à-vis someone's personal 
preferences/priorities, but I'd argue it's objectively better from a technical 
perspective, as it keeps strictly more information as otherwise. On top of that 
we're solving (b) for those who do not want to make use of this additional 
information, or are bothered by it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132486

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

[PATCH] D132486: Revert "libclang.so: Make SONAME the same as LLVM version"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

IMO looks good, thanks!

For reference & completeness, this came from 
https://github.com/h-vetinari/llvm-project/tree/libclang after discussion in 
https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132486

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

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D129160#3716826 , @isuruf wrote:

> Maybe this should be a cmake option that is off by default and I can turn it 
> for my builds?

I proposed something along those lines in 
https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129160

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

[PATCH] D123967: Disable update_cc_test_checks.py tests in stand-alone builds

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123967

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/docs/CPlusPlus20Modules.rst:64-67
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and 
``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. The dot ``.`` in 
the name has
+no special meaning.
+

h-vetinari wrote:
> 
This is marked as done, but is still unchanged in D131388


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

https://reviews.llvm.org/D131062

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

[PATCH] D131388: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Thanks! Repeating a point that might have been overlooked from D131062 
 (this time not as comments in the diff to 
avoid the "pollution" that caused the move to this PR):

> If you do open a new revision, please also consider breaking the lines at a 
> length that phabricator doesn't overflow (seems to be 122 characters), and 
> change all occurrences of "codes" to "code".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131388

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/docs/CPlusPlus20Modules.rst:676-678
+So the final answer for why we don't reuse the interface of Clang modules for 
header units is that
+we've see some differences between header units and Clang modules and we think 
the differences may
+be too large to be acceptable in the future.

ChuanqiXu wrote:
> h-vetinari wrote:
> > 
> Since it says `in the future`, if it is better to use `may be` or `will be` 
> than `are` ?
> Since it says `in the future`, if it is better to use `may be` or `will be` 
> than `are` ?

The "we think" already contains built-in subjectiveness, so the "may be" is 
redundant in terms of uncertainty. It's not a big deal, but in general: either 
"they may be" or "we think they are".

It would also be possible to say something like:

> So the final answer for why we don't reuse the interface of Clang modules for 
> header units is that
> there are some differences between header units and Clang modules and that 
> ignoring those
> differences now would likely become a problem in the future.



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

https://reviews.llvm.org/D131062

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

If you do open a new revision, please also consider breaking the lines at a 
length that phabricator doesn't overflow (seems to be 122 characters), and 
change all occurrences of "codes" to "code".




Comment at: clang/docs/CPlusPlus20Modules.rst:676-678
+So the final answer for why we don't reuse the interface of Clang modules for 
header units is that
+we've see some differences between header units and Clang modules and we think 
the differences may
+be too large to be acceptable in the future.




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

https://reviews.llvm.org/D131062

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D131062#3705864 , @ChuanqiXu wrote:

> Yeah, I am also worrying if the comments may block other reviewers to review. 
> Do you know any method to ignore these comments in phab? If not, I guess I 
> need to create a new page (but this is what we want?)

I don't know phab well at all, but I understand that old review comments remain 
is a feature and not a bug. But purely for pragmatic reasons already, it might 
be better to open a new revision.


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

https://reviews.llvm.org/D131062

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

It gets very confusing that phab now attaches the old review comments in the 
wrong place.




Comment at: clang/docs/CPlusPlus20Modules.rst:22
+different semantics, it might be more friendly for users who care about C++20
+modules only to create a new page.
+

OK that's fine. Then this should be changed from
> Due to the C++20 modules having very different semantics, it might be more 
> friendly for users who care about C++20 modules only to create a new page.

to something like

> Due to the C++20 modules having very different semantics, this page deals 
> with them separately.



Comment at: clang/docs/CPlusPlus20Modules.rst:52
+``Clang module map modules`` or ``Clang c++ modules``.
+
+Module and module unit

I had corrected myself on second pass, but got confused with phab and ended up 
not submitting it



Comment at: clang/docs/CPlusPlus20Modules.rst:64-67
+Terms enclosed in ``[]`` are optional. The syntax of ``module_name`` and 
``partition_name``
+in regex form corresponds to ``[a-zA-Z_][a-zA-Z_0-9\.]*``. The dot ``.`` in 
the name has
+no special meaning.
+





Comment at: clang/docs/CPlusPlus20Modules.rst:233-235
+The file name of module files should end with ``.pcm``.
+The file name of the module file of a ``primary module interface unit`` should 
be ``module_name.pcm``.
+The file name of module files of ``module partition unit`` should be 
``module_name-partition_name.pcm``.

> The comes from the clang implementation. If the user don't follow the 
> restrictions, then the clang may fail to build the module. For example, in 
> the "hello world" example, if the name of module file is M.module instead of 
> M.pcm, the the clang would fail to find the corresponding M module.

Can we add a sentence to this effect?



Comment at: clang/docs/CPlusPlus20Modules.rst:316-317
+
+Currently Clang would accept the above example. But it may produce surprising 
results if the
+debugging code depends on consistent use of ``NDEBUG`` also in other 
translation units.
+

I realized afterwards that it's the inverse - debugging code might depend on 
the _absence_  of `NDEBUG` which switches of debug stuff.



Comment at: clang/docs/CPlusPlus20Modules.rst:471
+This couldn't be demangled by previous versions of the debugger or demangler.
+As of LLVM 15.x, user can utilize ``llvm-cxxfilt`` to demangle this:
+





Comment at: clang/docs/CPlusPlus20Modules.rst:49-51
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. It is also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.

h-vetinari wrote:
> 
Ugh, this should have been "These are".


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

https://reviews.llvm.org/D131062

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

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

> It would be greatly welcome for such comments!

OK, here goes. Sorry for the large volume of comments. In addition to typos and 
stylistic improvements, I've had a few questions where the content wasn't clear 
to me (but note I'm not experienced with modules at all, so this may be obvious 
to others). I've also added a few line breaks where Phab was awkwardly 
overflowing. The position of the linebreaks is obviously irrelevant, but I 
thought it might help further review.




Comment at: clang/docs/CPlusPlus20Modules.rst:11
+
+Modules have a lot of meanings. For the users of Clang compiler, modules may
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header 
Modules``,





Comment at: clang/docs/CPlusPlus20Modules.rst:13-14
+refer to ``Objective-C Modules``, ``Clang C++ Modules`` (or ``Clang Header 
Modules``,
+etc) and C++20 modules. The implementation of all kinds of the modules in 
Clang 
+share a big part of codes. But from the perspective of users, their semantics 
and
+command line interfaces are very different. So it should be helpful for the 
users





Comment at: clang/docs/CPlusPlus20Modules.rst:15-16
+share a big part of codes. But from the perspective of users, their semantics 
and
+command line interfaces are very different. So it should be helpful for the 
users
+to introduce how to use C++20 modules.
+

Not 100% what the intention of the last sentence is - I presume it sets the 
goal for this document?



Comment at: clang/docs/CPlusPlus20Modules.rst:20-21
+should be helpful to read `Clang modules `_ if you want to know
+more about the general idea of modules. Due to the C++20 modules having very
+different semantics, it might be more friendly for users who care about C++20
+modules only to create a new page.

> Due to the C++20 modules having very different semantics, it might be more 
> friendly for users who care about C++20 modules only to create a new page.

Isn't "C++20 modules" what this page intends to do? Which new page are we 
talking about then?



Comment at: clang/docs/CPlusPlus20Modules.rst:24-26
+Although the term ``modules`` has a unique meaning in C++20 Language 
Specification,
+when people talk about C++20 modules, they may refer to another C++20 feature:
+header units. So this document would try to cover header units too.





Comment at: clang/docs/CPlusPlus20Modules.rst:31-35
+This document was intended to be pure manual. But it should be helpful to
+introduce some language background here for readers who are not familiar with
+the new language feature. This document is not intended to be a language
+tutorial. The document would only introduce concepts about the the
+structure and building of the project.





Comment at: clang/docs/CPlusPlus20Modules.rst:49-51
+In this document, the term ``Clang Modules``/``Clang modules`` refer to Clang
+c++ modules extension. It is also known as ``Clang header modules``,
+``Clang module map modules`` or ``Clang c++ modules``.





Comment at: clang/docs/CPlusPlus20Modules.rst:64-66
+Things in ``[]`` means optional. The syntax of ``module_name`` and 
``partition_name``
+in regex form should be ``[a-zA-Z_][a-zA-Z_0-9.]*``. The dot ``.`` in the name 
has
+no special meaning.

> The dot ``.`` in the name has no special meaning.

Not sure if this is intended to say that the dot in the regex is not needed, or 
that it has no semantic significance.

Also, when wanting to match a literal "." in a regex, I'd consider it 
beneficial for clarity to escape it ("\."), even though it does what's intended 
in the context of [].



Comment at: clang/docs/CPlusPlus20Modules.rst:78-80
+A primary module interface unit is a module unit whose module declaration is
+``export module module_name;``. The ``module_name`` here denotes the name of 
the
+module. A module should have one and only primary module interface unit.





Comment at: clang/docs/CPlusPlus20Modules.rst:94-98
+In this document, we call ``primary module interface unit`` and
+``module partition interface unit`` as ``module interface unit``. We call 
``module
+interface unit`` and ``module partition implementation unit`` as
+``importable module unit``. We call ``module partition interface unit`` and
+``module partition implementation unit`` as ``module partition unit``.

This seems quite important for the terminology of the rest of the document, so 
I'd structure it in a way that stand out visually, e.g. the suggestion above.



Comment at: clang/docs/CPlusPlus20Modules.rst:147
+
+Then let's see a little bit more complex HelloWorld example which uses the 4 
kinds of module units.
+

Missing 

[PATCH] D131062: [docs] Add "C++20 Modules"

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Aside from a couple typos, I was wondering if it would be welcome to do a pass 
w.r.t stylistic improvements (e.g. "Modules have a lot of meanings." --> "The 
term 'modules' has a lot of meanings"). I don't want to be too nit-picky - the 
sentences are all understandable, but sometimes slightly unusual to a native 
speaker in their formulation.


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

https://reviews.llvm.org/D131062

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

[PATCH] D130827: [clang] Fixed a number of typos

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Thanks for chasing these down!




Comment at: clang/include/clang/Basic/SourceManager.h:1922
+/// SourceManager and necessary dependencies (e.g. VFS, FileManager) for a
+/// single in-memorty file.
 class SourceManagerForFile {

Even though that line was touched, "in-memorty" seems to have managed to stay 
under the radar. One for the next round. ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130827

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

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

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

My point boils down to: "written using standard C++17
code" does not sound at all like "core language, no stdlib", but very much like 
"core+stdlib".

This is also the first time this split becomes relevant AFAIK, because for 
moving to C++14, the stdlib was ready basically around the same time / compiler 
versions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

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

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

From the text you quoted:

> LLVM subprojects are written using standard C++17

code and avoid unnecessary vendor-specific extensions.

I don't think the standard library can be called a vendor-specific extension, 
and so I think this still could/should be made clearer (even though the status 
links below would show upon inspection that there's no full support yet, but 
that's a bit too tucked away I feel).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

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

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

It may be worth calling out that this is about C++17 core language and not the 
standard library?

libstdcxx only finished C++17 support in GCC 12, and libcxx is still missing 
various pieces even today (much less for Clang 5).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

In D129160#3678431 , @tstellar wrote:

> @h-vetinari Does the release note look OK?

Basically yes, thank you! I'd still be more precise and say "soversion" rather 
than "soname", but otherwise OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129160

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

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Is it realistic for this to land before LLVM 15 branches? Would be great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189

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

[PATCH] D128619: [Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:494-497
+- Implemented "Conditionally Trivial Special Member Functions" (`P0848 
`_).
+  Note: The handling of deleted functions is not yet compliant, as Clang
+  does not implement `DR1496 
`_
+  and `DR1734 
`_.

Is that lack of compliance worth a note in `cxx_status`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128619

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

[PATCH] D123967: Disable update_cc_test_checks.py tests in stand-alone builds

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Just comments, looks OK otherwise.




Comment at: clang/test/CMakeLists.txt:6
   CLANG_BUILD_EXAMPLES
+  CLANG_BUILT_STANDALONE
   CLANG_DEFAULT_PIE_ON_LINUX

OT for this PR, but it would be nice to eventually rename this to 
`CLANG_BUILD_STANDALONE` in accordance with the present tense used for other 
variables.



Comment at: clang/test/lit.site.cfg.py.in:40
 config.llvm_external_lit = path(r"@LLVM_EXTERNAL_LIT@")
+config.stand_alone_build = @CLANG_BUILT_STANDALONE@
 

I'd use one spelling of standalone / stand-alone, and I think in this case it's 
probably the env var that wins?



Comment at: clang/test/utils/update_cc_test_checks/lit.local.cfg:19
+# for any of the clang source code.
+config.unsupported = True
 else:

I couldn't tell from the diff where this is used (though admittedly I hardly 
know the LLVM infra). I also don't see it in [[ 
https://github.com/llvm/llvm-project/blob/main/clang/test/lit.cfg.py | 
`lit.cfg.py` ]], the respective [[ 
https://github.com/llvm/llvm-project/blob/main/clang/test/CMakeLists.txt | 
`CMakeLists.txt` ]] or [[ 
https://github.com/llvm/llvm-project/blob/llvmorg-14.0.6/llvm/cmake/modules/AddLLVM.cmake#L1616
 | `configure_lit_site_cfg` ]]

I trust that this does what's intended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123967

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

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:472-476
+- Implemented `P1103R3: Merging Modules `_.
+- Implemented `P1779R3: ABI isolation for member functions 
`_.
+- Implemented `P1874R1: Dynamic Initialization Order of Non-Local Variables in 
Modules `_.
+- Partially implemented `P1815R2: Translation-unit-local entities 
`_.
+

This should probably also be reflected in 
https://clang.llvm.org/cxx_status.html / 
https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_status.html ...?


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

https://reviews.llvm.org/D129138

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

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Forgot to mention it before: would be good to note this in the release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129160

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

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Thanks for the ping. It's unfortunate that this didn't work out as intended. 
It's certainly fine though (for our purposes) to go back to the way things were 
before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129160

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Very happy to see this finally happening! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D122008: [flang][driver] Add support for generating executables

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Speaking as a relative outside (but who's been waiting for flang in LLVM since 
well before it joined the monorepo), I'd much prefer 
code-generation-with-rough-edges in LLVM 15 (to start testing and raising 
eventual bugs), rather than a more polished flang (realistically still with 
bugs that need to be shaken out) in LLVM 16.

I can understand why Option 2 would be more attractive for those not wanting to 
attract too much attention yet (and shouldn't be a blocker for my purposes), 
hence I looked a bit into:

In D122008#3448529 , @awarzynski 
wrote:

> I investigated a bit and discovered that internally CMake calls try_compile 
> 
>  (CMake docs ). 
> I don't see  any way to pass custom compiler options there (i.e. 
> `try_compile` expects `flang-new file.f90` to just work). This makes me 
> rather hesitant to pursue Option 2 at all.

I'd be more than a little surprised that there's no way to do this. Have you 
tried CMAKE_Fortran_FLAGS 
? AFAICT, 
the environment variables should be respected. After a bit of googling, a very 
similar issue came up 
  
already at least 15 years ago, and here's a recent SO answer 
 
about it. Worst case, one can always ask on the CMake discourse 
, people are very helpful and responsive in my 
experience.

Given that Option 2 might be the path of highest consensus, I think it would be 
worth a bit more investigation if it's really impossible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122008

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

[PATCH] D118352: [clang][ABI] New c++20 modules mangling scheme

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Something must have gone wrong... communication-wise... as @urnathan seems to 
have abandoned (resp. resigned from) all modules PRs. Hope any 
misunderstandings or grievances can be worked out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118352

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

[PATCH] D118599: [C++20][Modules][8/8] Amend module visibility rules for partitions.

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

A non-actionable comment out of curiosity for the work




Comment at: clang/test/Modules/cxx20-10-1-ex2.cpp:15-17
-// Not expected to work yet.
 //  %clang_cc1 -std=c++20 -emit-module-interface %t/std10-1-ex2-tu4.cpp \
 //   -fmodule-file=%t/B.pcm  -o %t/B_X2.pcm

Should these two tests now be switched on? Missing a `RUN: ` for that IIUC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118599

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