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.

Details

Reviewers
jansvoboda11
Summary

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

Diff Detail

Unit TestsFailed

TimeTest
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