[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+    llvm::outs() << Opts.MainFileName << "\n";
----------------
steveire wrote:
> hans wrote:
> > zturner wrote:
> > > steveire wrote:
> > > > I'll have to keep a patch around anyway for myself locally to remove this condition. But maybe someone else will benefit from this.
> > > > 
> > > > What do you think about changing CMake to so that this is passed by default when using Clang-CL?
> > > > 
> > > Do you mean llvms cmake?  Or cmake itself?  Which generator are you using?
> > Can't you just configure your build to pass the flag to clang-cl?
> > 
> > I suppose CMake could be made to do that by default when using clang-cl versions that support the flag and using the MSBuild generator, but that's for the CMake community to decide I think. Or perhaps our VS integration could do that, but it gets tricky because it needs to know whether the flag is supported or not.
> I mean upstream cmake. My suggestion is independent of the generator, but it would depend on what Brad decides anyway. I don't intend to implement it, just report it.
> 
> I only have one clang but I have multiple buildsystems, and I don't want to have to add a new flag too all of them. In each toplevel CMakeLists everyone will need this to make use of the flag:
> 
> ```
> 
> # Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
> # Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
> # variable named "MSVC" and
> # the way CMake variables and the "if()" command work requires this. 
> if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
>         AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
>     # CMake does not have explicit Clang-CL support yet.
>     # It should have a COMPILER_ID for it.
>     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
>     # etc
> endif()
> ```
> 
> Upstream CMake can have the logic to add the flag instead.
But then what if you don’t want it?  There would be no way to turn it off.  I don’t think it’s a good fit for cmake 


================
Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+    llvm::outs() << Opts.MainFileName << "\n";
----------------
zturner wrote:
> steveire wrote:
> > hans wrote:
> > > zturner wrote:
> > > > steveire wrote:
> > > > > I'll have to keep a patch around anyway for myself locally to remove this condition. But maybe someone else will benefit from this.
> > > > > 
> > > > > What do you think about changing CMake to so that this is passed by default when using Clang-CL?
> > > > > 
> > > > Do you mean llvms cmake?  Or cmake itself?  Which generator are you using?
> > > Can't you just configure your build to pass the flag to clang-cl?
> > > 
> > > I suppose CMake could be made to do that by default when using clang-cl versions that support the flag and using the MSBuild generator, but that's for the CMake community to decide I think. Or perhaps our VS integration could do that, but it gets tricky because it needs to know whether the flag is supported or not.
> > I mean upstream cmake. My suggestion is independent of the generator, but it would depend on what Brad decides anyway. I don't intend to implement it, just report it.
> > 
> > I only have one clang but I have multiple buildsystems, and I don't want to have to add a new flag too all of them. In each toplevel CMakeLists everyone will need this to make use of the flag:
> > 
> > ```
> > 
> > # Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
> > # Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
> > # variable named "MSVC" and
> > # the way CMake variables and the "if()" command work requires this. 
> > if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
> >         AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
> >     # CMake does not have explicit Clang-CL support yet.
> >     # It should have a COMPILER_ID for it.
> >     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
> >     # etc
> > endif()
> > ```
> > 
> > Upstream CMake can have the logic to add the flag instead.
> But then what if you don’t want it?  There would be no way to turn it off.  I don’t think it’s a good fit for cmake 
Yes, and actually for this reason i was wondering if we can do it without a flag at all.  What if we just set an magic environment variable?  It handles Stephen’s use case (he can just set it globally), and MSBuild (we can set it in the extension).


https://reviews.llvm.org/D52773