Page MenuHomePhabricator

[HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.
ClosedPublic

Authored by vsapsai on Jun 15 2021, 7:59 PM.

Details

Summary

There is a separate field isPragmaOnce and when isImport combines
both, it complicates HeaderFileInfo serialization as #pragma once is
the inherent property of the header while isImport reflects how other
headers use it. The usage of the header can be different in different
contexts, that's why isImport requires tracking separate from #pragma once.

Diff Detail

Event Timeline

vsapsai created this revision.Jun 15 2021, 7:59 PM
vsapsai requested review of this revision.Jun 15 2021, 7:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2021, 7:59 PM
vsapsai added inline comments.Jun 15 2021, 8:07 PM
clang/include/clang/Lex/HeaderSearch.h
455–458

This affects the result of hasFileBeenImported that is used only in Indexing.cpp as

bool isParsedOnceInclude(const FileEntry *FE) {
  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE) ||
         PP.getHeaderSearchInfo().hasFileBeenImported(FE);
}

and isFileMultipleIncludeGuarded is checking isPragmaOnce already, so no behavior change as far as I can tell.

This seems reasonable on first look. Can you add a test that demonstrates the problem this patch solves?

This seems reasonable on first look. Can you add a test that demonstrates the problem this patch solves?

There is no externally observable change that can be tested, I'm only making sure the existing tests keep passing. In the subsequent commit I change the restoration of isImport flag from .pcm because it reflects how a header is used, not an inherent property of a header that stays the same regardless of the header usage. To make things easier to implement I've decided to separate these two qualities of a header. And I think it fits names better, like hasFileBeenImported. With this change we don't need to add in the comments that it means hasFileBeenImportedOrHasPragmaOnce.

jansvoboda11 accepted this revision.Aug 6 2021, 4:43 AM

Ok, this makes sense. LGTM.

This revision is now accepted and ready to land.Aug 6 2021, 4:43 AM

Thanks for the review! I'll wait with committing this change until https://reviews.llvm.org/D104344 is ready to avoid bumping VERSION_MAJOR twice. While this change doesn't introduce any syntactical differences to serialized AST, it does introduce semantic differences.