[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D122983#3427518 <https://reviews.llvm.org/D122983#3427518>, @xbolva00 wrote:

> Could you please check that https://github.com/llvm/llvm-test-suite is buildable with your patch?

I gave it a shot just to see, but I'm unable to build it even without my patch:

  F:\source\test-suite-build>cmake --build . --clean-first
  Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
  Copyright (C) Microsoft Corporation. All rights reserved.
  
  F:\source\test-suite-build\test-suite.sln : Solution file error MSB5004: The solution file has two projects named "pa
  thfinder".
  Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
  Copyright (C) Microsoft Corporation. All rights reserved.
  
    [TEST_SUITE_HOST_CC] Compiling host source fpcmp.c
    'cc' is not recognized as an internal or external command,
    operable program or batch file.
  C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.target
  s(241,5): error MSB8066: Custom build for 'F:\source\test-suite-build\CMakeFiles\a30a18e8f3bb5de504f740ee3822d8a0\fpc
  mp.c.o.rule;F:\source\test-suite-build\CMakeFiles\a30a18e8f3bb5de504f740ee3822d8a0\fpcmp.rule;F:\source\test-suite-bu
  ild\CMakeFiles\22099cfe59041de58024b8c82a571139\build-fpcmp.rule' exited with code 9009. [F:\source\test-suite-build\
  tools\build-fpcmp.vcxproj]
    [TEST_SUITE_HOST_CC] Compiling host source timeit.c
    'cc' is not recognized as an internal or external command,
    operable program or batch file.
  C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.target
  s(241,5): error MSB8066: Custom build for 'F:\source\test-suite-build\CMakeFiles\a30a18e8f3bb5de504f740ee3822d8a0\tim
  eit.c.o.rule;F:\source\test-suite-build\CMakeFiles\a30a18e8f3bb5de504f740ee3822d8a0\timeit.rule;F:\source\test-suite-
  build\CMakeFiles\22099cfe59041de58024b8c82a571139\build-timeit.rule' exited with code 9009. [F:\source\test-suite-bui
  ld\tools\build-timeit.vcxproj]
  cl : command line warning D9025: overriding '/W4' with '/w' [F:\source\test-suite-build\MicroBenchmarks\libs\benchmar
  k\third_party\googletest\build\googlemock\gmock_main.vcxproj]
  cl : command line error D8021: invalid numeric argument '/Werror=date-time' [F:\source\test-suite-build\MicroBenchmar
  ks\libs\benchmark\third_party\googletest\build\googlemock\gmock_main.vcxproj]
  cl : command line warning D9025: overriding '/W3' with '/w' [F:\source\test-suite-build\tools\fpcmp-target.vcxproj]
  cl : command line error D8021: invalid numeric argument '/Werror=date-time' [F:\source\test-suite-build\tools\fpcmp-t
  arget.vcxproj]
    Generating sqlite test inputs
    'TCL_TCLSH-NOTFOUND' is not recognized as an internal or external command,
    operable program or batch file.
  C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.target
  s(241,5): error MSB8066: Custom build for 'F:\source\test-suite-build\CMakeFiles\47602658443eaf701f82747ba4e9979c\tes
  t15.sql.rule' exited with code 9009. [F:\source\test-suite-build\MultiSource\Applications\sqlite3\sqlite_input.vcxpro
  j]
  cl : command line warning D9025: overriding '/W3' with '/w' [F:\source\test-suite-build\tools\timeit-target.vcxproj]
  cl : command line error D8021: invalid numeric argument '/Werror=date-time' [F:\source\test-suite-build\tools\timeit-
  target.vcxproj]

FWIW, this was how I configured, and that step did not generate any errors for me:

  cmake -DCMAKE_C_COMPILER=F:\source\llvm-project\llvm\out\build\x64-Debug\bin\clang.exe -DCMAKE_CXX_COMPILER="F:\source\llvm-project\llvm\out\build\x64-Debug\bin\clang++.exe" -C..\test-suite\cmake\caches\O3.cmake -DTEST_SUITE_COLLECT_CODE_SIZE=OFF ..\test-suite

So I'm not certain what's going on there.

In D122983#3427677 <https://reviews.llvm.org/D122983#3427677>, @hubert.reinterpretcast wrote:

> In D122983#3426716 <https://reviews.llvm.org/D122983#3426716>, @erichkeane wrote:
>
>> We typically avoid doing -verify in CodeGen (unless the diagnostic is ACTUALLY in CodeGen) as a matter of business.
>
> I hope that `-verify` and `// expected-no-diagnostics` in CodeGen tests is compatible with the above. I believe it is valuable to confirm that the test itself is not written problematically.

Morally, yes, that's reasonable in CodeGen because you're ensuring you get no diagnostics. Practically, that's a convoluted, more expensive, less maintainable way to spell `-Werror` for the test. When diagnostics are introduced, this pattern encourages people to remove the `// expected-no-diagnostics` comment and start adding `// expected-warning {{}}` comments. Running the diagnostic verifier also slows down test execution because of the extra verification step (which adds up over thousands of tests).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122983/new/

https://reviews.llvm.org/D122983