Hi Richard, This commit makes one of the tests fail in: https://reviews.llvm.org/D27689
The implicit modules model expects to be able to "upgrade" an implicit PCM with more -Werror flags *without* affecting the signature. However, with your commit, the command-line diagnostic flags (e.g., -Wconversion vs -Wno-conversion) now change the AST, and thus change the ASTFileSignature. One possible hammer would be to disable this pragma-diagnostic-stuff somehow for implicit modules. Do you have any other suggestions? Thanks, Duncan > On 2017-Jan-25, at 17:01, Richard Smith via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > Author: rsmith > Date: Wed Jan 25 19:01:01 2017 > New Revision: 293123 > > URL: http://llvm.org/viewvc/llvm-project?rev=293123&view=rev > Log: > Remove and replace DiagStatePoint tracking and lookup data structure. > > Rather than storing a single flat list of SourceLocations where the diagnostic > state changes (in source order), we now store a separate list for each FileID > in which there is a diagnostic state transition. (State for other files is > built and cached lazily, on demand.) This has two consequences: > > 1) We can now sensibly support modules, and properly track the diagnostic > state > for modular headers (this matters when, for instance, triggering instantiation > of a template defined within a module triggers diagnostics). > > 2) It's much faster than the old approach, since we can now just do a binary > search on the offsets within the FileID rather than needing to call > isBeforeInTranslationUnit to determine source order (which is surprisingly > slow). For some pathological (but real world) files, this reduces total > compilation time by more than 10%. > > For now, the diagnostic state points for modules are loaded eagerly. It seems > feasible to defer this until diagnostic state information for one of the > module's files is needed, but that's not part of this patch. > > Added: > cfe/trunk/test/Modules/diag-pragma.cpp > Modified: > cfe/trunk/include/clang/Basic/Diagnostic.h > cfe/trunk/lib/Basic/Diagnostic.cpp > cfe/trunk/lib/Basic/DiagnosticIDs.cpp > cfe/trunk/lib/Serialization/ASTReader.cpp > cfe/trunk/lib/Serialization/ASTWriter.cpp > cfe/trunk/test/Modules/Inputs/diag_pragma.h > > Modified: cfe/trunk/include/clang/Basic/Diagnostic.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Basic/Diagnostic.h (original) > +++ cfe/trunk/include/clang/Basic/Diagnostic.h Wed Jan 25 19:01:01 2017 > @@ -29,6 +29,7 @@ > #include <cassert> > #include <cstdint> > #include <list> > +#include <map> > #include <memory> > #include <string> > #include <type_traits> > @@ -232,40 +233,97 @@ private: > /// \brief Keeps and automatically disposes all DiagStates that we create. > std::list<DiagState> DiagStates; > > - /// \brief Represents a point in source where the diagnostic state was > - /// modified because of a pragma. > - /// > - /// 'Loc' can be null if the point represents the diagnostic state > - /// modifications done through the command-line. > - struct DiagStatePoint { > - DiagState *State; > - SourceLocation Loc; > - DiagStatePoint(DiagState *State, SourceLocation Loc) > - : State(State), Loc(Loc) { } > + /// A mapping from files to the diagnostic states for those files. Lazily > + /// built on demand for files in which the diagnostic state has not > changed. > + class DiagStateMap { > + public: > + /// Add an initial diagnostic state. > + void appendFirst(DiagState *State); > + /// Add a new latest state point. > + void append(SourceManager &SrcMgr, SourceLocation Loc, DiagState *State); > + /// Look up the diagnostic state at a given source location. > + DiagState *lookup(SourceManager &SrcMgr, SourceLocation Loc) const; > + /// Determine whether this map is empty. > + bool empty() const { return Files.empty(); } > + /// Clear out this map. > + void clear() { > + Files.clear(); > + FirstDiagState = CurDiagState = nullptr; > + CurDiagStateLoc = SourceLocation(); > + } > + > + /// Grab the most-recently-added state point. > + DiagState *getCurDiagState() const { return CurDiagState; } > + /// Get the location at which a diagnostic state was last added. > + SourceLocation getCurDiagStateLoc() const { return CurDiagStateLoc; } > + > + private: > + /// \brief Represents a point in source where the diagnostic state was > + /// modified because of a pragma. > + /// > + /// 'Loc' can be null if the point represents the diagnostic state > + /// modifications done through the command-line. > + struct DiagStatePoint { > + DiagState *State; > + unsigned Offset; > + DiagStatePoint(DiagState *State, unsigned Offset) > + : State(State), Offset(Offset) { } > + }; > + > + /// Description of the diagnostic states and state transitions for a > + /// particular FileID. > + struct File { > + /// The diagnostic state for the parent file. This is strictly > redundant, > + /// as looking up the DecomposedIncludedLoc for the FileID in the Files > + /// map would give us this, but we cache it here for performance. > + File *Parent = nullptr; > + /// The offset of this file within its parent. > + unsigned ParentOffset = 0; > + /// Whether this file has any local (not imported from an AST file) > + /// diagnostic state transitions. > + bool HasLocalTransitions = false; > + /// The points within the file where the state changes. There will > always > + /// be at least one of these (the state on entry to the file). > + llvm::SmallVector<DiagStatePoint, 4> StateTransitions; > + > + DiagState *lookup(unsigned Offset) const; > + }; > + > + /// The diagnostic states for each file. > + mutable std::map<FileID, File> Files; > + > + /// The initial diagnostic state. > + DiagState *FirstDiagState; > + /// The current diagnostic state. > + DiagState *CurDiagState; > + /// The location at which the current diagnostic state was established. > + SourceLocation CurDiagStateLoc; > + > + /// Get the diagnostic state information for a file. > + File *getFile(SourceManager &SrcMgr, FileID ID) const; > + > + friend class ASTReader; > + friend class ASTWriter; > }; > > - /// \brief A sorted vector of all DiagStatePoints representing changes in > - /// diagnostic state due to diagnostic pragmas. > - /// > - /// The vector is always sorted according to the SourceLocation of the > - /// DiagStatePoint. > - typedef std::vector<DiagStatePoint> DiagStatePointsTy; > - mutable DiagStatePointsTy DiagStatePoints; > + DiagStateMap DiagStatesByLoc; > > /// \brief Keeps the DiagState that was active during each diagnostic 'push' > /// so we can get back at it when we 'pop'. > std::vector<DiagState *> DiagStateOnPushStack; > > DiagState *GetCurDiagState() const { > - assert(!DiagStatePoints.empty()); > - return DiagStatePoints.back().State; > + return DiagStatesByLoc.getCurDiagState(); > } > > void PushDiagStatePoint(DiagState *State, SourceLocation L); > > /// \brief Finds the DiagStatePoint that contains the diagnostic state of > /// the given source location. > - DiagStatePointsTy::iterator GetDiagStatePointForLoc(SourceLocation Loc) > const; > + DiagState *GetDiagStateForLoc(SourceLocation Loc) const { > + return SourceMgr ? DiagStatesByLoc.lookup(*SourceMgr, Loc) > + : DiagStatesByLoc.getCurDiagState(); > + } > > /// \brief Sticky flag set to \c true when an error is emitted. > bool ErrorOccurred; > @@ -372,7 +430,7 @@ public: > return *SourceMgr; > } > void setSourceManager(SourceManager *SrcMgr) { > - assert(DiagStatePoints.size() == 1 && DiagStatePoints[0].Loc.isInvalid() > && > + assert(DiagStatesByLoc.empty() && > "Leftover diag state from a different SourceManager."); > SourceMgr = SrcMgr; > } > > Modified: cfe/trunk/lib/Basic/Diagnostic.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Diagnostic.cpp?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/lib/Basic/Diagnostic.cpp (original) > +++ cfe/trunk/lib/Basic/Diagnostic.cpp Wed Jan 25 19:01:01 2017 > @@ -132,13 +132,13 @@ void DiagnosticsEngine::Reset() { > > // Clear state related to #pragma diagnostic. > DiagStates.clear(); > - DiagStatePoints.clear(); > + DiagStatesByLoc.clear(); > DiagStateOnPushStack.clear(); > > // Create a DiagState and DiagStatePoint representing diagnostic changes > // through command-line. > DiagStates.emplace_back(); > - DiagStatePoints.emplace_back(&DiagStates.back(), SourceLocation()); > + DiagStatesByLoc.appendFirst(&DiagStates.back()); > } > > void DiagnosticsEngine::SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1, > @@ -158,49 +158,94 @@ void DiagnosticsEngine::ReportDelayed() > DelayedDiagArg2.clear(); > } > > +void DiagnosticsEngine::DiagStateMap::appendFirst( > + DiagState *State) { > + assert(Files.empty() && "not first"); > + FirstDiagState = CurDiagState = State; > + CurDiagStateLoc = SourceLocation(); > +} > + > +void DiagnosticsEngine::DiagStateMap::append(SourceManager &SrcMgr, > + SourceLocation Loc, > + DiagState *State) { > + CurDiagState = State; > + CurDiagStateLoc = Loc; > + > + std::pair<FileID, unsigned> Decomp = SrcMgr.getDecomposedLoc(Loc); > + unsigned Offset = Decomp.second; > + for (File *F = getFile(SrcMgr, Decomp.first); F; > + Offset = F->ParentOffset, F = F->Parent) { > + F->HasLocalTransitions = true; > + auto &Last = F->StateTransitions.back(); > + assert(Last.Offset <= Offset && "state transitions added out of order"); > + > + if (Last.Offset == Offset) { > + if (Last.State == State) > + break; > + Last.State = State; > + continue; > + } > + > + F->StateTransitions.push_back({State, Offset}); > + } > +} > + > +DiagnosticsEngine::DiagState * > +DiagnosticsEngine::DiagStateMap::lookup(SourceManager &SrcMgr, > + SourceLocation Loc) const { > + // Common case: we have not seen any diagnostic pragmas. > + if (Files.empty()) > + return FirstDiagState; > + > + std::pair<FileID, unsigned> Decomp = SrcMgr.getDecomposedLoc(Loc); > + const File *F = getFile(SrcMgr, Decomp.first); > + return F->lookup(Decomp.second); > +} > + > +DiagnosticsEngine::DiagState * > +DiagnosticsEngine::DiagStateMap::File::lookup(unsigned Offset) const { > + auto OnePastIt = std::upper_bound( > + StateTransitions.begin(), StateTransitions.end(), Offset, > + [](unsigned Offset, const DiagStatePoint &P) { > + return Offset < P.Offset; > + }); > + assert(OnePastIt != StateTransitions.begin() && "missing initial state"); > + return OnePastIt[-1].State; > +} > + > +DiagnosticsEngine::DiagStateMap::File * > +DiagnosticsEngine::DiagStateMap::getFile(SourceManager &SrcMgr, > + FileID ID) const { > + // Get or insert the File for this ID. > + auto Range = Files.equal_range(ID); > + if (Range.first != Range.second) > + return &Range.first->second; > + auto &F = Files.insert(Range.first, std::make_pair(ID, File()))->second; > + > + // We created a new File; look up the diagnostic state at the start of it > and > + // initialize it. > + if (ID.isValid()) { > + std::pair<FileID, unsigned> Decomp = SrcMgr.getDecomposedIncludedLoc(ID); > + F.Parent = getFile(SrcMgr, Decomp.first); > + F.ParentOffset = Decomp.second; > + F.StateTransitions.push_back({F.Parent->lookup(Decomp.second), 0}); > + } else { > + // This is the (imaginary) root file into which we pretend all top-level > + // files are included; it descends from the initial state. > + // > + // FIXME: This doesn't guarantee that we use the same ordering as > + // isBeforeInTranslationUnit in the cases where someone invented another > + // top-level file and added diagnostic pragmas to it. See the code at the > + // end of isBeforeInTranslationUnit for the quirks it deals with. > + F.StateTransitions.push_back({FirstDiagState, 0}); > + } > + return &F; > +} > + > void DiagnosticsEngine::PushDiagStatePoint(DiagState *State, > SourceLocation Loc) { > - // Make sure that DiagStatePoints is always sorted according to Loc. > assert(Loc.isValid() && "Adding invalid loc point"); > - assert(!DiagStatePoints.empty() && > - (DiagStatePoints.back().Loc.isInvalid() || > - getSourceManager().isBeforeInTranslationUnit( > - DiagStatePoints.back().Loc, Loc)) && > - "Previous point loc comes after or is the same as new one"); > - DiagStatePoints.push_back(DiagStatePoint(State, Loc)); > -} > - > -DiagnosticsEngine::DiagStatePointsTy::iterator > -DiagnosticsEngine::GetDiagStatePointForLoc(SourceLocation L) const { > - assert(!DiagStatePoints.empty()); > - assert(DiagStatePoints.front().Loc.isInvalid() && > - "Should have created a DiagStatePoint for command-line"); > - > - if (!SourceMgr) > - return DiagStatePoints.end() - 1; > - > - FullSourceLoc Loc(L, *SourceMgr); > - if (Loc.isInvalid()) > - return DiagStatePoints.end() - 1; > - > - DiagStatePointsTy::iterator Pos = DiagStatePoints.end(); > - SourceLocation LastStateChangePos = DiagStatePoints.back().Loc; > - if (LastStateChangePos.isValid() && > - Loc.isBeforeInTranslationUnitThan(LastStateChangePos)) > - Pos = std::upper_bound( > - DiagStatePoints.begin(), DiagStatePoints.end(), > - DiagStatePoint(nullptr, Loc), > - [this](const DiagStatePoint &LHS, const DiagStatePoint &RHS) { > - // If Loc is invalid it means it came from <command-line>, in which > - // case we regard it as coming before any valid source location. > - if (RHS.Loc.isInvalid()) > - return false; > - if (LHS.Loc.isInvalid()) > - return true; > - return SourceMgr->isBeforeInTranslationUnit(LHS.Loc, RHS.Loc); > - }); > - --Pos; > - return Pos; > + DiagStatesByLoc.append(*SourceMgr, Loc, State); > } > > void DiagnosticsEngine::setSeverity(diag::kind Diag, diag::Severity Map, > @@ -210,11 +255,8 @@ void DiagnosticsEngine::setSeverity(diag > assert((Diags->isBuiltinWarningOrExtension(Diag) || > (Map == diag::Severity::Fatal || Map == diag::Severity::Error)) && > "Cannot map errors into warnings!"); > - assert(!DiagStatePoints.empty()); > assert((L.isInvalid() || SourceMgr) && "No SourceMgr for valid location"); > > - SourceLocation Loc = SourceMgr ? L : SourceLocation(); > - SourceLocation LastStateChangePos = DiagStatePoints.back().Loc; > // Don't allow a mapping to a warning override an error/fatal mapping. > if (Map == diag::Severity::Warning) { > DiagnosticMapping &Info = GetCurDiagState()->getOrAddMapping(Diag); > @@ -225,49 +267,22 @@ void DiagnosticsEngine::setSeverity(diag > DiagnosticMapping Mapping = makeUserMapping(Map, L); > > // Common case; setting all the diagnostics of a group in one place. > - if (Loc.isInvalid() || Loc == LastStateChangePos) { > - GetCurDiagState()->setMapping(Diag, Mapping); > - return; > - } > - > - // Another common case; modifying diagnostic state in a source location > - // after the previous one. > - if ((Loc.isValid() && LastStateChangePos.isInvalid()) || > - SourceMgr->isBeforeInTranslationUnit(LastStateChangePos, Loc)) { > - // A diagnostic pragma occurred, create a new DiagState initialized with > - // the current one and a new DiagStatePoint to record at which location > - // the new state became active. > - DiagStates.push_back(*GetCurDiagState()); > - PushDiagStatePoint(&DiagStates.back(), Loc); > - GetCurDiagState()->setMapping(Diag, Mapping); > - return; > - } > - > - // We allow setting the diagnostic state in random source order for > - // completeness but it should not be actually happening in normal practice. > - > - DiagStatePointsTy::iterator Pos = GetDiagStatePointForLoc(Loc); > - assert(Pos != DiagStatePoints.end()); > - > - // Update all diagnostic states that are active after the given location. > - for (DiagStatePointsTy::iterator > - I = Pos+1, E = DiagStatePoints.end(); I != E; ++I) { > - I->State->setMapping(Diag, Mapping); > - } > - > - // If the location corresponds to an existing point, just update its state. > - if (Pos->Loc == Loc) { > - Pos->State->setMapping(Diag, Mapping); > + if ((L.isInvalid() || L == DiagStatesByLoc.getCurDiagStateLoc()) && > + DiagStatesByLoc.getCurDiagState()) { > + // FIXME: This is theoretically wrong: if the current state is shared > with > + // some other location (via push/pop) we will change the state for that > + // other location as well. This cannot currently happen, as we can't > update > + // the diagnostic state at the same location at which we pop. > + DiagStatesByLoc.getCurDiagState()->setMapping(Diag, Mapping); > return; > } > > - // Create a new state/point and fit it into the vector of DiagStatePoints > - // so that the vector is always ordered according to location. > - assert(SourceMgr->isBeforeInTranslationUnit(Pos->Loc, Loc)); > - DiagStates.push_back(*Pos->State); > - DiagState *NewState = &DiagStates.back(); > - NewState->setMapping(Diag, Mapping); > - DiagStatePoints.insert(Pos + 1, DiagStatePoint(NewState, Loc)); > + // A diagnostic pragma occurred, create a new DiagState initialized with > + // the current one and a new DiagStatePoint to record at which location > + // the new state became active. > + DiagStates.push_back(*GetCurDiagState()); > + DiagStates.back().setMapping(Diag, Mapping); > + PushDiagStatePoint(&DiagStates.back(), L); > } > > bool DiagnosticsEngine::setSeverityForGroup(diag::Flavor Flavor, > > Modified: cfe/trunk/lib/Basic/DiagnosticIDs.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/DiagnosticIDs.cpp?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/lib/Basic/DiagnosticIDs.cpp (original) > +++ cfe/trunk/lib/Basic/DiagnosticIDs.cpp Wed Jan 25 19:01:01 2017 > @@ -411,11 +411,8 @@ DiagnosticIDs::getDiagnosticSeverity(uns > // to error. Errors can only be mapped to fatal. > diag::Severity Result = diag::Severity::Fatal; > > - DiagnosticsEngine::DiagStatePointsTy::iterator > - Pos = Diag.GetDiagStatePointForLoc(Loc); > - DiagnosticsEngine::DiagState *State = Pos->State; > - > // Get the mapping information, or compute it lazily. > + DiagnosticsEngine::DiagState *State = Diag.GetDiagStateForLoc(Loc); > DiagnosticMapping &Mapping = State->getOrAddMapping((diag::kind)DiagID); > > // TODO: Can a null severity really get here? > > Modified: cfe/trunk/lib/Serialization/ASTReader.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) > +++ cfe/trunk/lib/Serialization/ASTReader.cpp Wed Jan 25 19:01:01 2017 > @@ -5297,48 +5297,84 @@ HeaderFileInfo ASTReader::GetHeaderFileI > } > > void ASTReader::ReadPragmaDiagnosticMappings(DiagnosticsEngine &Diag) { > - // FIXME: Make it work properly with modules. > - SmallVector<DiagnosticsEngine::DiagState *, 32> DiagStates; > + using DiagState = DiagnosticsEngine::DiagState; > + SmallVector<DiagState *, 32> DiagStates; > + > for (ModuleIterator I = ModuleMgr.begin(), E = ModuleMgr.end(); I != E; > ++I) { > ModuleFile &F = *(*I); > unsigned Idx = 0; > + auto &Record = F.PragmaDiagMappings; > + if (Record.empty()) > + continue; > + > DiagStates.clear(); > - assert(!Diag.DiagStates.empty()); > - DiagStates.push_back(&Diag.DiagStates.front()); // the command-line one. > - while (Idx < F.PragmaDiagMappings.size()) { > - SourceLocation Loc = ReadSourceLocation(F, > F.PragmaDiagMappings[Idx++]); > - unsigned DiagStateID = F.PragmaDiagMappings[Idx++]; > - if (DiagStateID != 0) { > - Diag.DiagStatePoints.push_back( > - > DiagnosticsEngine::DiagStatePoint(DiagStates[DiagStateID-1], > - FullSourceLoc(Loc, SourceMgr))); > - continue; > - } > > - assert(DiagStateID == 0); > + auto ReadDiagState = > + [&](const DiagState &BasedOn, SourceLocation Loc, > + bool IncludeNonPragmaStates) -> DiagnosticsEngine::DiagState * { > + unsigned BackrefID = Record[Idx++]; > + if (BackrefID != 0) > + return DiagStates[BackrefID - 1]; > + > // A new DiagState was created here. > - Diag.DiagStates.push_back(*Diag.GetCurDiagState()); > - DiagnosticsEngine::DiagState *NewState = &Diag.DiagStates.back(); > + Diag.DiagStates.push_back(BasedOn); > + DiagState *NewState = &Diag.DiagStates.back(); > DiagStates.push_back(NewState); > - Diag.DiagStatePoints.push_back( > - DiagnosticsEngine::DiagStatePoint(NewState, > - FullSourceLoc(Loc, SourceMgr))); > - while (true) { > - assert(Idx < F.PragmaDiagMappings.size() && > - "Invalid data, didn't find '-1' marking end of diag/map > pairs"); > - if (Idx >= F.PragmaDiagMappings.size()) { > - break; // Something is messed up but at least avoid infinite loop > in > - // release build. > - } > - unsigned DiagID = F.PragmaDiagMappings[Idx++]; > - if (DiagID == (unsigned)-1) { > - break; // no more diag/map pairs for this location. > - } > - diag::Severity Map = (diag::Severity)F.PragmaDiagMappings[Idx++]; > + while (Idx + 1 < Record.size() && Record[Idx] != unsigned(-1)) { > + unsigned DiagID = Record[Idx++]; > + diag::Severity Map = (diag::Severity)Record[Idx++]; > DiagnosticMapping Mapping = Diag.makeUserMapping(Map, Loc); > - Diag.GetCurDiagState()->setMapping(DiagID, Mapping); > + if (Mapping.isPragma() || IncludeNonPragmaStates) > + NewState->setMapping(DiagID, Mapping); > + } > + assert(Idx != Record.size() && Record[Idx] == unsigned(-1) && > + "Invalid data, didn't find '-1' marking end of diag/map pairs"); > + ++Idx; > + return NewState; > + }; > + > + auto *FirstState = ReadDiagState( > + F.isModule() ? DiagState() : *Diag.DiagStatesByLoc.CurDiagState, > + SourceLocation(), F.isModule()); > + SourceLocation CurStateLoc = > + ReadSourceLocation(F, F.PragmaDiagMappings[Idx++]); > + auto *CurState = ReadDiagState(*FirstState, CurStateLoc, false); > + > + if (!F.isModule()) { > + Diag.DiagStatesByLoc.CurDiagState = CurState; > + Diag.DiagStatesByLoc.CurDiagStateLoc = CurStateLoc; > + > + // Preserve the property that the imaginary root file describes the > + // current state. > + auto &T = Diag.DiagStatesByLoc.Files[FileID()].StateTransitions; > + if (T.empty()) > + T.push_back({CurState, 0}); > + else > + T[0].State = CurState; > + } > + > + while (Idx < Record.size()) { > + SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]); > + auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc); > + assert(IDAndOffset.second == 0 && "not a start location for a FileID"); > + unsigned Transitions = Record[Idx++]; > + > + // Note that we don't need to set up Parent/ParentOffset here, because > + // we won't be changing the diagnostic state within imported FileIDs > + // (other than perhaps appending to the main source file, which has no > + // parent). > + auto &F = Diag.DiagStatesByLoc.Files[IDAndOffset.first]; > + F.StateTransitions.reserve(F.StateTransitions.size() + Transitions); > + for (unsigned I = 0; I != Transitions; ++I) { > + unsigned Offset = Record[Idx++]; > + auto *State = > + ReadDiagState(*FirstState, Loc.getLocWithOffset(Offset), false); > + F.StateTransitions.push_back({State, Offset}); > } > } > + > + // Don't try to read these mappings again. > + Record.clear(); > } > } > > > Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original) > +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Jan 25 19:01:01 2017 > @@ -2802,38 +2802,43 @@ ASTWriter::inferSubmoduleIDFromLocation( > > void ASTWriter::WritePragmaDiagnosticMappings(const DiagnosticsEngine &Diag, > bool isModule) { > - // Make sure set diagnostic pragmas don't affect the translation unit that > - // imports the module. > - // FIXME: Make diagnostic pragma sections work properly with modules. > - if (isModule) > - return; > - > llvm::SmallDenseMap<const DiagnosticsEngine::DiagState *, unsigned, 64> > DiagStateIDMap; > unsigned CurrID = 0; > - DiagStateIDMap[&Diag.DiagStates.front()] = ++CurrID; // the command-line > one. > RecordData Record; > - for (DiagnosticsEngine::DiagStatePointsTy::const_iterator > - I = Diag.DiagStatePoints.begin(), E = Diag.DiagStatePoints.end(); > - I != E; ++I) { > - const DiagnosticsEngine::DiagStatePoint &point = *I; > - if (point.Loc.isInvalid()) > - continue; > > - AddSourceLocation(point.Loc, Record); > - unsigned &DiagStateID = DiagStateIDMap[point.State]; > + auto AddDiagState = [&](const DiagnosticsEngine::DiagState *State, > + bool IncludeNonPragmaStates) { > + unsigned &DiagStateID = DiagStateIDMap[State]; > Record.push_back(DiagStateID); > - > + > if (DiagStateID == 0) { > DiagStateID = ++CurrID; > - for (const auto &I : *(point.State)) { > - if (I.second.isPragma()) { > + for (const auto &I : *State) { > + if (I.second.isPragma() || IncludeNonPragmaStates) { > Record.push_back(I.first); > Record.push_back((unsigned)I.second.getSeverity()); > } > } > - Record.push_back(-1); // mark the end of the diag/map pairs for this > - // location. > + // Add a sentinel to mark the end of the diag IDs. > + Record.push_back(unsigned(-1)); > + } > + }; > + > + AddDiagState(Diag.DiagStatesByLoc.FirstDiagState, isModule); > + AddSourceLocation(Diag.DiagStatesByLoc.CurDiagStateLoc, Record); > + AddDiagState(Diag.DiagStatesByLoc.CurDiagState, false); > + > + for (auto &FileIDAndFile : Diag.DiagStatesByLoc.Files) { > + if (!FileIDAndFile.first.isValid() || > + !FileIDAndFile.second.HasLocalTransitions) > + continue; > + > AddSourceLocation(Diag.SourceMgr->getLocForStartOfFile(FileIDAndFile.first), > + Record); > + Record.push_back(FileIDAndFile.second.StateTransitions.size()); > + for (auto &StatePoint : FileIDAndFile.second.StateTransitions) { > + Record.push_back(StatePoint.Offset); > + AddDiagState(StatePoint.State, false); > } > } > > > Modified: cfe/trunk/test/Modules/Inputs/diag_pragma.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/diag_pragma.h?rev=293123&r1=293122&r2=293123&view=diff > ============================================================================== > --- cfe/trunk/test/Modules/Inputs/diag_pragma.h (original) > +++ cfe/trunk/test/Modules/Inputs/diag_pragma.h Wed Jan 25 19:01:01 2017 > @@ -1,3 +1,13 @@ > #define DIAG_PRAGMA_MACRO 1 > > #pragma clang diagnostic ignored "-Wparentheses" > + > +#ifdef __cplusplus > +template<typename T> const char *f(T t) { > + return "foo" + t; > +} > +#pragma clang diagnostic ignored "-Wstring-plus-int" > +template<typename T> const char *g(T t) { > + return "foo" + t; > +} > +#endif > > Added: cfe/trunk/test/Modules/diag-pragma.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/diag-pragma.cpp?rev=293123&view=auto > ============================================================================== > --- cfe/trunk/test/Modules/diag-pragma.cpp (added) > +++ cfe/trunk/test/Modules/diag-pragma.cpp Wed Jan 25 19:01:01 2017 > @@ -0,0 +1,47 @@ > +// RUN: rm -rf %t > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -emit-module > -fmodules-cache-path=%t -fmodule-name=diag_pragma -x c++ %S/Inputs/module.map > -fmodules-ts > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -verify > -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -emit-module > -fmodule-name=diag_pragma -x c++ %S/Inputs/module.map -fmodules-ts -o > %t/explicit.pcm -Werror=string-plus-int > +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -verify > -fmodules-cache-path=%t -I %S/Inputs %s -fmodules-ts -DEXPLICIT_FLAG > -fmodule-file=%t/explicit.pcm > + > +import diag_pragma; > + > +int foo(int x) { > + // Diagnostics from templates in the module follow the diagnostic state > from > + // when the module was built. > +#ifdef EXPLICIT_FLAG > + // expected-error@diag_pragma.h:7 {{adding 'int' to a string}} > +#else > + // expected-warning@diag_pragma.h:7 {{adding 'int' to a string}} > +#endif > + // expected-note@diag_pragma.h:7 {{use array indexing}} > + f(0); // expected-note {{instantiation of}} > + > + g(0); // ok, warning was ignored when building module > + > + // Diagnostics from this source file ignore the diagnostic state from the > + // module. > + void("foo" + x); // expected-warning {{adding 'int' to a string}} > + // expected-note@-1 {{use array indexing}} > + > +#pragma clang diagnostic ignored "-Wstring-plus-int" > + > + // Diagnostics from the module ignore diagnostic state changes from this > + // source file. > +#ifdef EXPLICIT_FLAG > + // expected-error@diag_pragma.h:7 {{adding 'long' to a string}} > +#else > + // expected-warning@diag_pragma.h:7 {{adding 'long' to a string}} > +#endif > + // expected-note@diag_pragma.h:7 {{use array indexing}} > + f(0L); // expected-note {{instantiation of}} > + > + g(0L); > + > + void("bar" + x); > + > + if (x = DIAG_PRAGMA_MACRO) // expected-warning {{using the result of an > assignment as a condition without parentheses}} \ > + // expected-note {{place parentheses}} > expected-note {{use '=='}} > + return 0; > + return 1; > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits