@sammccall was there any progress on this so far? This would be really useful to be a clang-tidy check.

In D88458#3117882, @phosek wrote:

In D88458#3117828, @Abpostelnicu wrote:

In D88458#3117814, @phosek wrote:

It looks to me like you're using the pre-monorepo layout where you move compiler-rt to llvm/projects, is that correct?

Yes, you are correct.

Is there any particular reason for doing this rather than simply using the monorepo layout?

We could make it work, but I'm not sure if we should. The monorepo layout is the only supported one and we've already discussed the plans to remove the non-monorepo layout support to simplify our build.

It would be better to make it work, removing support for the old build system will break many toolchains.

So far you're the only one who reached affected by this change which to me suggests that there may not be many projects using the old layout.

When LLVM adopted the monorepo we made no guarantees that we would keep the old layout working.

True but this is the first change that breaks it.

We're also planning larger build changes after LLVM 14 which will likely disrupt you, see https://lists.llvm.org/pipermail/llvm-dev/2021-October/153238.html.

I think we are getting a bit far, there is time till then to do our adjustments.

In D88458#3117814, @phosek wrote:

In D88458#3117759, @Abpostelnicu wrote:

Sure, is this something that helps https://paste.mozilla.org/yxUYUBk3 ?

Yes, thank you. The issue is this:

CMake Error at projects/compiler-rt/cmake/Modules/AddCompilerRT.cmake:3 (include):
include could not find load file:

/builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/../cmake/Modules/HandleCompilerRT.cmake
Call Stack (most recent call first):
projects/compiler-rt/lib/CMakeLists.txt:4 (include)

It looks to me like you're using the pre-monorepo layout where you move compiler-rt to llvm/projects, is that correct?

Yes, you are correct.

We could make it work, but I'm not sure if we should. The monorepo layout is the only supported one and we've already discussed the plans to remove the non-monorepo layout support to simplify our build.

It would be better to make it work, removing support for the old build system will break many toolchains.

In D88458#3109346, @phosek wrote:

In D88458#3108920, @Abpostelnicu wrote:

In D88458#3100557, @phosek wrote:

In D88458#3100139, @Abpostelnicu wrote:

In D88458#3095459, @phosek wrote:

In D88458#3093023, @Abpostelnicu wrote:

In D88458#3089403, @phosek wrote:

Properly handle the flags to address the issue seen on some bots when cross-compiling.

I don't know if you noticed my previous comment but this again broke the build. It's a tar of the build directory.

I tried reproducing the issue locally using the command you provided but I'm not seeing the same error. Would it be possible to share CMakeFiles/CMakeError.log from your build?

Thanks for looking into this. Please see this.

Thanks, I looked at the CMakeError.log file but there's nothing out of ordinary. I don't think that the failing histedit.h check is a new issue, that seems like the expected behavior.

Can I see the full build log somewhere?

Please let me know if this helps https://drive.google.com/file/d/1nzKVJOnYWuXKH1nJufClXCq5QHeJBlVp/view?usp=sharing

I was hoping to see the output from CMake which is not included.

In D88458#3100557, @phosek wrote:

In D88458#3100139, @Abpostelnicu wrote:

In D88458#3095459, @phosek wrote:

In D88458#3093023, @Abpostelnicu wrote:

In D88458#3089403, @phosek wrote:

Properly handle the flags to address the issue seen on some bots when cross-compiling.

I don't know if you noticed my previous comment but this again broke the build. It's a tar of the build directory.

I tried reproducing the issue locally using the command you provided but I'm not seeing the same error. Would it be possible to share CMakeFiles/CMakeError.log from your build?

Thanks for looking into this. Please see this.

Thanks, I looked at the CMakeError.log file but there's nothing out of ordinary. I don't think that the failing histedit.h check is a new issue, that seems like the expected behavior.

Can I see the full build log somewhere?

Please let me know if this helps https://drive.google.com/file/d/1nzKVJOnYWuXKH1nJufClXCq5QHeJBlVp/view?usp=sharing

In D88458#3095459, @phosek wrote:

In D88458#3093023, @Abpostelnicu wrote:

In D88458#3089403, @phosek wrote:

Properly handle the flags to address the issue seen on some bots when cross-compiling.

I don't know if you noticed my previous comment but this again broke the build.

I tried reproducing the issue locally using the command you provided but I'm not seeing the same error. Would it be possible to share CMakeFiles/CMakeError.log from your build?

In D88458#3089403, @phosek wrote:

Properly handle the flags to address the issue seen on some bots when cross-compiling.

I think this creates a build bustage when llvm is built with the following flags:

In D102233#3048032, @gchatelet wrote:

In D102233#3047627, @Abpostelnicu wrote:

Using clang-13, that has this changeset I encounter: clang-13: error: the clang compiler does not support '-march=native'.

Thx for reporting @Abpostelnicu . Which CPU are you compiling for?

Using clang-13, that has this changeset I encounter: clang-13: error: the clang compiler does not support '-march=native'.

In D105207#2909571, @chrisjackson wrote:

Thanks for the reports, I have reverted and will look into the crash ASAP.

I think this introduced a build crash on Firefox. I'm saying this because the build crash occurs in the function touched by this patch, and the yesterday build, that didn't have this patch worked just fine!

In D100581#2798079, @raj.khem wrote:

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable]
    unsigned int size0, size1, supp_size = 0;

You have, with pragma gcc error, look at hb.hh. The problem is that clang has an error dealing with cascading pragma when setting different levels.

I think there is a false positive with this @george.burgess.iv:
In this we have the warning triggered:
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' set but not used [-Werror,-Wunused-but-set-variable]

In D100625#2741385, @hvdijk wrote:

Apparently Phabricator detected the presence of "revert D100625" in my alternative D101851's description and marked that as reverting this one. Of course, as this hadn't been committed, it cannot have been reverted either. Could you confirm that things work for you now, @Abpostelnicu?

In D100625#2697345, @jpalus wrote:

Do you mind taking a look at fix proposed in:

https://bugs.llvm.org/show_bug.cgi?id=49990#c3

Sorry for not noticing this before, this is pretty much what I came up with too and what's committed now.

I'm seeing here something very strange, if the user provided copy assignment operator is provided but is = delete and the copy constructor is default this warning will still trigger. Is this something expected?
I'm referring at this issue from the mozilla code-base.

Thank you for this! Also I think it’s very well that you want to implement this warning in clang, with a little bit of more polish it will be up for merger!

In D100581#2723611, @lebedev.ri wrote:

In D100581#2723505, @Abpostelnicu wrote:

Also I don’t remember seeing this proposed on cfe dev mailing list.

There is no such requirement. I don't recall that happening basically ever, actually.

Please revert this. This change hasn’t been tested nor reviewed enough. Also I don’t remember seeing this proposed on cfe dev mailing list.

I think this added a regression, where you have this context:

In D100625#2694236, @kadircet wrote:

In D100625#2694191, @Abpostelnicu wrote:

As you can see I don't specify LLVM_ENABLE_PROJECTS but still looking at the build failure, it seems that clang-tools-extra is being built, or parts of it are:

because I don't specify LLVM_ENABLE_PROJECTS so the set will not occur.

but the sample you provided has -DLLVM_ENABLE_PROJECTS=clang;compiler-rt in it, hence it doesn't really reproduce the issue.

In my case, this will never be true but, because I don't specify LLVM_ENABLE_PROJECTS so the set will not occur.

In D100625#2694014, @kadircet wrote:

If you don't specify LLVM_ENABLE_PROJECTS clang-tools-extra will still be built but without having set LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR.

This feels like the failure of the first case i mentioned, i.e. some cmake file under CTE being executed even though it is disabled. Can you provide some repro steps? For example when I do:

git clone git@github.com:llvm/llvm-project.git
cd llvm-project && mkdir build_test && cd build_test
cmake -G Ninja ../llvm/ -DCMAKE_BUILD_TYPE="RELEASE"
ninja all

this finishes without any errors. as expected clang-tools-extra/clangd/quality/CompletionModel.cmake doesn't interfere with the build when clang-tools-extra is not mentioned in LLVM_ENABLE_PROJECTS.

---

also there are lots of build bots building different subsets of LLVM in different configurations and none of them have been broken by this change. so it would be great to understand the reason behind the breakage you are experiencing.

In D100625#2693971, @kadircet wrote:

I am not an expert on Cmake nor understand what is going on here fully, but here are my 2 cents.

D58157 didn't remove LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR, it just stopped enabling CTE whenever clang was enabled.
All of LLVM_EXTERNAL_$PROJECT variables are set based on the values passed into LLVM_ENABLE_PROJECTS in https://github.com/llvm/llvm-project/blob/main/llvm/CMakeLists.txt#L105.

So in theory this cmake file should only be evaluated when CTE is enabled and LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR should always be set when CTE is enabled.
The correct fix would be to figure out why one of the above statements are not holding, i.e.:

• This cmake file is evaluated/executed despite CTE being off (this shouldn't be the case)
• LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR is not set, even though CTE is part of LLVM_ENABLE_PROJECTS.

Add -use-color argument to make it sane with clang-tidy --use-color argument.

This has landed and trunk is already fixed.

Landed in 11.1 https://github.com/llvm/llvm-project/commit/1fdec59bffc11ae37eb51a1b9869f0696bfd5312#diff-2fa23ad0cf1839955ddaf4a0d78a9d9b5fd9b88933f82f6433035916a2655c6c.
Patch needs to be rebased for trunk now.

The failure is wrong.

rebased on 11.1rc2

@tstellar what do you suggest to do here?

In D89156#2530839, @tstellar wrote:

In D89156#2530703, @Abpostelnicu wrote:

In D89156#2530701, @JosephTremoulet wrote:

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

I have a fix for this ready, I can push it to 11.0.1.

Can you file a bug for this or re-open the merge request bug?

Update for plularization of variables name.

Update with format

And there are more issues like: https://github.com/llvm/llvm-project/blob/llvmorg-11.0.1/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp#L733. old_module_sp_ptr is nowhere declared.

In D89156#2530701, @JosephTremoulet wrote:

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

In D89156#2530656, @JosephTremoulet wrote:

In D89156#2530353, @Abpostelnicu wrote:

Shouldn't this also update PlatformiOSSimulator.h?

There is no PlatformOSSimulator.h file (or I can't find it). There is a type PlatformiOSSumulator, but it doesn't have its own GetSharedModule method. Its CreateInstance method returns a PlatformAppleSimulator[1], and this change does update PlatformAppleSimulator. So unless I'm misunderstanding your point, I don't think any changes specific to PlatformiOSSimulator are needed

1 - https://github.com/llvm/llvm-project/blob/82847436e9258a12503dcfadb5dc373cb42fea43/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp#L538

Shouldn't this also update PlatformiOSSimulator.h?

In D79388#2321231, @MyDeveloperDay wrote:

Not to my knowledge, if we are not going to fix it we should probably revert it for now, we can bring it back later

Has been pushed to the 11.x branch https://github.com/llvm/llvm-project/commit/1596c2dfd548b21cf33ad3353882ac465d78c1bb

In D77882#2274292, @hans wrote:

This seems to have caused https://bugs.llvm.org/show_bug.cgi?id=47512

Can you take a look?

In D81917#2127690, @njames93 wrote:

In D81917#2127477, @Abpostelnicu wrote:

@njames93 wdyt if we add another parameter to distinguish if we want to use regex or not, and if not we escape the paths?
Also thank you so much for catching this up!

As the argument is documented as being a regex string, we shouldn't really try to escape it, and users should be expected escape any file names themselves before invoking the script.
If for whatever reason you want to use .c++ as your file extension. then you should pass r'*\.c\+\+' to the script.

Can I ask what the specific use case you have that is causing the issue? Are you invoking run-clang-tidy from a script that generates the list of files to search for?

@njames93 wdyt if we add another parameter to distinguish if we want to use regex or not, and if not we escape the paths?
Also thank you so much for catching this up!

Let's first see we don't break anything on mozilla.

Please see https://firefox-source-docs.mozilla.org/code-quality/coding-style/format_cpp_code_with_clang-format.html

@MyDeveloperDay thanks for the patch, I'm gonna run it agains mozilla to see if there is any fallout.

Sorry, totally forgot about this, thank you!

As per what @sammccall said.

Just to clarify something here, for me the patch looks good but until I will accept the revision I want to test it on the mozilla codebase.

In D78879#2004856, @MyDeveloperDay wrote:

@sylvestre.ledru , @Abpostelnicu

I believe to fix your original request you need a combination of D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107) and this fix (this fix will handle the * * issue in gecko-dev/ipc/mscom/Ptr.h

I've run this new binary over gecko-dev/ipc/mscom and gecko-dev/xpcom/ds and it shows no clang-format warnings

I hope this helps

I've cherry-picked D76850 to 10.x and see if this fixes the issue.

In D78879#2004724, @MyDeveloperDay wrote:

@sylvestre.ledru

I'm taking a quick look at formatting the original bug in gecko and whilst the last windows snapshot (Feb 2020) shows the bug

$ clang-format --version
clang-format version 11.0.0

nsTArray.h:939:29: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const nsTArray<E>&() {
                            ^
nsTArray.h:946:35: warning: code should be clang-formatted [-Wclang-format-violations]
  operator const FallibleTArray<E>&() {
                                  ^
nsTArray.h:1147:59: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray_Impl<E, Allocator>&() const& {
                                                          ^
nsTArray.h:1151:43: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const nsTArray<E>&() const& {
                                          ^
nsTArray.h:1154:49: warning: code should be clang-formatted [-Wclang-format-violations]
  [[nodiscard]] operator const FallibleTArray<E>&() const& {

The current trunk does not

$ clang-format --version
clang-format version 11.0.0 (https://github.com/llvm/llvm-project 1956a8a7cb79e94dbe073e36eba2d6b003f91046)

clang-format -n nsTArray.h

Is Mozilla toolchain using LLVM from the v10 branch?

I think this is fixed by D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

I think we are on the right track with this but what if we are dealing with tok::amp in a context similar to operator const FallibleTArray<E>&(); I can speculate that the outcome will be operator const FallibleTArray<E> &(); which is wrong.

In D77882#1976216, @sylvestre.ledru wrote:

please add this to the release notes too :)
something like "new option -allow-enabling-alpha-checkers added to run-clang-tidy to enable alpha checkers"

Add release notes.

done

In D74031#1859231, @hans wrote:

Looks great, thanks!

Do you have commit access? If so, go ahead and push directly to the 10.x branch, otherwise let me know and I'll do it for you.

Yes I have commit access, pushing it directly to 10.x then.

Adding extra comments to the release patch.

In D74031#1859084, @hans wrote:

lgtm, but if you want it wouldn't hurt to include even more details from the commit message.

In D72438#1811512, @lebedev.ri wrote:

Hm, didn't clang gain such a diagnostic itself recently? https://godbolt.org/z/MYJTvw
Wouldn't it make sense to migrate everything into it, and drop this now-duplicating check?