On Wed, 2025-06-11 at 11:57 -0400, Jason Merrill wrote: > On 6/3/25 7:02 PM, David Malcolm wrote: > > On Sat, 2025-05-31 at 23:25 -0400, Jason Merrill wrote: > > > + if (kind == DK_POP) > > > + opt += offset; > > > > I'm wondering why the offset is applied to "opt" here? That feels > > wrong to me, or, at least, I couldn't see what it's trying to do, > > which > > got me thinking about the DK_POP case. > > DK_POP returns to the state at the specified index: > > > /* For DK_POP, this is the index of the corresponding push (as > > stored > > in > > m_push_list). > > > > Otherwise, this is an option index. */ > > int option; > > ...and since we're appending to the vector here, the index in the > interface TU needs to be adjusted for any entries that might have > already been in the vector before we started appending. I've added > more > comments. > > > Does diagnostic_option_classifier's m_push_list get updated when > > reading classification changes from the module? > > That's not necessary; m_push_list is only for remembering the index > we > want to return to when we later see a pop, which is not relevant to > an > importer. > > > What happens if you have a module that pushes a diagnostic > > classification change, and later pops it? (analogous to a header > > that > > temporarily suppresses a warning, if I understand correctly) If > > that's > > reasonable to do, it would be good to have a test case for it, > > since I > > don't think the existing test cases are covering the DK_POP case. > > There's a lot of push/pop in the libstdc++ headers that this change > was > motivated by, including <initializer_list> that's included by the new > test. So when writing out the changes from warn-spec-3_a.C we have > > Diagnostic changes: 8 > Index 0: ignored: -Wvariadic-macros > Index 1: ignored: -Wc++23-extensions > Index 2: pop from 0 > Index 3: ignored: -Wvariadic-macros > Index 4: ignored: -Wc++11-extensions > Index 5: ignored: -Wc++23-extensions > Index 6: pop from 3 > Index 7: ignored: -Winit-list-lifetime > > > Probably less reasonable: what if a module pushes a diagnostics > > classification change, but *doesn't* later pop it. Can the change > > be > > popped by consumers of the module? > > That's not something we want to support; importing a module should > not > change the diagnostic configuration in the importer. > > But yes, the reader should make sure that any changes are popped at > the > end of an import. Fixed, and tested in warn-spec-3_c.C. > > This revision also avoids re-exporting imported entries and adds > detail > to -fdump-lang-module. > > Jason
Thanks for the updates. This version looks good to me. Dave
