In https://reviews.llvm.org/D102861 changes to the LNT suite were made to accommodate the clang patch in https://reviews.llvm.org/D74436 . These changes were made in the CMakeLists.txt. Unfortunately the buildbot seems to be running make. This patch is to add those same (FLAGS) changes in D102861 to the Makefiles.
Details
Diff Detail
Event Timeline
LGTM - please see changes requested inline. thanks a lot
SingleSource/Benchmarks/Misc-C++/Large/Makefile | ||
---|---|---|
3 | I think these should all say "+=" instead of merely "=" because there might be an initialization of flags from somewhere else e.g. the Make command line invocation. If you use += then it will add onto the flags, whereas straight assignment I believe would lose the other setting. If there's no particular reason why some files use CPPFLAGS and others divide into CXXFLAGS and CFLAGS why not use the same setting in each file (i.e. choose one way or the other) I'm going to go ahead and +1 this assuming you will make these changes LGTM thanks so much for doing this! |
SingleSource/Benchmarks/Misc-C++/Large/Makefile | ||
---|---|---|
3 | I will change all these to +=. |
SingleSource/Benchmarks/Misc-C++/Large/Makefile | ||
---|---|---|
3 | But then I see that MultiSource/Makefile.multisrc also has a rule that doesn't use CPPFLAGS with CFLAGS (line 95). |
SingleSource/Benchmarks/Misc-C++/Large/Makefile | ||
---|---|---|
3 | Probably doesn't matter, just being picky. I'm good with whatever you choose. thanks again |
commit 79f2b03c5111392d647fa542e120f70c8f39a991
Author: Zahira Ammarguellat <zahira.ammarguellat@intel.com>
Date: Mon Jul 26 11:36:23 2021 -0700
Closed by this commit (fixed) the buildbots are working well for Intel thanks Zahira
I think these should all say "+=" instead of merely "=" because there might be an initialization of flags from somewhere else e.g. the Make command line invocation. If you use += then it will add onto the flags, whereas straight assignment I believe would lose the other setting.
If there's no particular reason why some files use CPPFLAGS and others divide into CXXFLAGS and CFLAGS why not use the same setting in each file (i.e. choose one way or the other)
I'm going to go ahead and +1 this assuming you will make these changes
LGTM thanks so much for doing this!