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

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://

[PATCH] D135341: adds `__reference_constructs_from_temporary`

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-vendo

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

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

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

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

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

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-com

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

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

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

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

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

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

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

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

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

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

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

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 r

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

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. -

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

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-befo

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

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 h

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

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/Driv

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

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

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

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

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

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

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

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/ http

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

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

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

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/projec

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

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. ---

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

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 (thou

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

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 cont

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

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 change

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

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

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

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 swit

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

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-w

[PATCH] D132486: SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

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 t

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

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 Rep

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

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-removin

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

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/

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

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 speci

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

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

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

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 differ

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

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-67

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

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 (

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

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 p

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

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, s

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

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 understandabl

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

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 tho

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

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 basic

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

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

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

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 C

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

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

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

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-co

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

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 impleme

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

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_BUIL

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

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 I

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

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

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

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/n

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

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-commit

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

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 (realistica

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

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:/

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

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 %