ro marked 5 inline comments as done. ro added a comment. In D64793#1604877 <https://reviews.llvm.org/D64793#1604877>, @jyknight wrote:
> > I fear it is necessary: at least it matches documented behaviour of both > > the Sun/Oracle Studio compilers and gcc. > > I will defer to your opinion here. But -- one last attempt at dissuading you. > :) > > Is this really doing something _important_, or is it just legacy cruft which > can be safely ignored by now? With your "logb" example, it seems to me that > it is probably preferable to always use the new correct "xpg6" > implementation, and just ignore the legacy/incorrect version. Similarly, the > example given in https://gcc.gnu.org/PR40411 of freopen -- again, seems like > it'd be better to just use the new xpg6 behavior, always. For new code, you're certainly right, but existing C90 code that relies on pre-C99 behaviour should receive correct results IMO. Otherwise, you could just as well remove C90 support in clang and argue that C99 (or even C11) is better ;-) >> The -std= options usually get passed to the linking step because CFLAGS is >> added to the options as well > > With gnu make they are not (unless it's doing a single-step compiling+linking > step). Other tools like CMake also don't pass standards versions to linking. > This makes sense, because a program can contain code compiled with multiple > standards versions, and multiple languages. Thus, I'd expect most users to > just get the default xpg6 and Xa objects, even if they do specify -std=c90 > for compilation. I don't really buy this: there are several cases that do need passing the same (or companion) options to both the compiler and linker. Think of building 32-bit code with a 64-bit-default compiler where you need to pass -m32 to both. Similarly for -pthread both during compilation (to define `_REENTRANT`) and linking (to link with `-lpthread`). There are tons of other cases where something like this is mandatory. Depending on the build environment, the exact method to achieve this will differ (like adding -m32 to $(CC)), but it most certainly can be done. It's true that mixing different standards version in the same executable is problematic to say the best, but that's a quirk of that Solaris method we can do nothing about: developers just need to be aware of the limitation. ================ Comment at: lib/Driver/ToolChains/Solaris.cpp:16 #include "clang/Driver/Options.h" +#include "clang/Frontend/LangStandard.h" #include "llvm/Option/ArgList.h" ---------------- rnk wrote: > ro wrote: > > jyknight wrote: > > > I'm not sure that's an acceptable dependency -- I think Driver probably > > > is not supposed to depend on Frontend. If so, I guess LangStandard should > > > move to clang/Basic/. Which also means moving InputKind from > > > clang/include/clang/Frontend/FrontendOptions.h. > > > > > > (Maybe someone else can weigh in on this question?) > > I wondered about this myself, including frontend code in the > > driver might be considered a layering violation. I certainly need > > guidance here what is and isn't acceptable here. > I see there are no other includes of Frontend from Driver, so I think > LangStandards* does need to move to Basic. The only piece of InputKind that's > needed is the language enum. I'm surprised there isn't already one somewhere > else, but if there isn't, I think it would be reasonable to define the input > kind languages in LangStandard.h and use them from FrontendOptions. I've now implemented that move as [[https://reviews.llvm.org/D65562]]. I'll submit a revised version of this patch shortly. ================ Comment at: lib/Frontend/LangStandards.cpp:31-37 Kind K = llvm::StringSwitch<Kind>(Name) #define LANGSTANDARD(id, name, lang, desc, features) \ .Case(name, lang_##id) +#define LANGSTANDARD_ALIAS(id, alias) \ + .Case(alias, lang_##id) #include "clang/Frontend/LangStandards.def" .Default(lang_unspecified); ---------------- rnk wrote: > I see that this code pattern is repeated in two other places, > lib/Tooling/InterpolatingCompilationDatabase.cpp and > lib/Frontend/CompilerInvocation.cpp. I think it would be good to factor out a > string-to-kind helper and use it in the three places. Also included in the [[ https://reviews.llvm.org/D6556 | move patch]]. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64793/new/ https://reviews.llvm.org/D64793 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
