Page MenuHomePhabricator

[clang] Add warning when `-include-pch` is passed multiple times
Needs ReviewPublic

Authored by keith on Nov 2 2020, 1:53 PM.
This revision needs review, but all reviewers have resigned.



Previously this argument passed multiple times would result in the first
being silently ignored.

Diff Detail

Unit TestsFailed

70 msx64 debian > Clang.PCH::multiple-include-pch.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -emit-pch -o /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/PCH/Output/multiple-include-pch.c.tmp1.pch /mnt/disks/ssd0/agent/llvm-project/clang/test/PCH/multiple-include-pch.c
220 msx64 windows > Clang.PCH::multiple-include-pch.c
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -emit-pch -o C:\ws\w16-1\llvm-project\premerge-checks\build\tools\clang\test\PCH\Output\multiple-include-pch.c.tmp1.pch C:\ws\w16-1\llvm-project\premerge-checks\clang\test\PCH\multiple-include-pch.c

Event Timeline

keith created this revision.Nov 2 2020, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 1:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
keith requested review of this revision.Nov 2 2020, 1:53 PM
keith updated this revision to Diff 319138.Jan 25 2021, 3:11 PM

Add group to warning

There are many options whether the latter one overrides the previous ones. How is this special?

keith added a comment.Jan 25 2021, 3:19 PM

Fair question! I think this case is a bit different since it's quite subtle. The strange thing here is that the header you're intending to provide a pch for can still be read successfully, but not getting the benefits of the pch that you were expecting without knowing about it.

Is there any reason that -include-pch shouldn't follow the precedent of -include, which can be used multiple times? If not, then the end goal should be to support multiple uses, but in the mean time a warning is helpful.

keith added a comment.Tue, Feb 9, 9:34 AM

Yea seems like a reasonable request, looking through the uses nothing immediately pops out as being not supporting multiple PCHs but I guess before I went down that path I'd want someone who knew the code better to agree on that

jansvoboda11 resigned from this revision.Fri, Mar 5, 1:30 AM