[llvm] r250929 - Silence Visual C++ warning in function summary parsing code (NFC)

[David Blaikie via llvm-commits]

On Wed, Oct 21, 2015 at 3:40 PM, Teresa Johnson <tejohnson at google.com>
wrote:

> On Wed, Oct 21, 2015 at 3:01 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Wed, Oct 21, 2015 at 2:52 PM, Teresa Johnson <tejohnson at google.com>
> > wrote:
> >>
> >> On Wed, Oct 21, 2015 at 12:49 PM, David Blaikie <dblaikie at gmail.com>
> >> wrote:
> >> >
> >> >
> >> > On Wed, Oct 21, 2015 at 12:33 PM, Aaron Ballman via llvm-commits
> >> > <llvm-commits at lists.llvm.org> wrote:
> >> >>
> >> >> On Wed, Oct 21, 2015 at 3:25 PM, Teresa Johnson via llvm-commits
> >> >> <llvm-commits at lists.llvm.org> wrote:
> >> >> > Author: tejohnson
> >> >> > Date: Wed Oct 21 14:25:14 2015
> >> >> > New Revision: 250929
> >> >> >
> >> >> > URL: http://llvm.org/viewvc/llvm-project?rev=250929&view=rev
> >> >> > Log:
> >> >> > Silence Visual C++ warning in function summary parsing code (NFC)
> >> >> >
> >> >> > Modified:
> >> >> >     llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> >> >> >
> >> >> > Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
> >> >> > URL:
> >> >> >
> >> >> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=250929&r1=250928&r2=250929&view=diff
> >> >> >
> >> >> >
> >> >> >
> ==============================================================================
> >> >> > --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
> >> >> > +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Oct 21
> >> >> > 14:25:14
> >> >> > 2015
> >> >> > @@ -5538,6 +5538,7 @@ std::error_code FunctionIndexBitcodeRead
> >> >> >    // importing is added so that it can be tested.
> >> >> >    SmallVector<uint64_t, 64> Record;
> >> >> >    switch (Stream.readRecord(Entry.ID, Record)) {
> >> >> > +    case bitc::FS_CODE_COMBINED_ENTRY:
> >> >> >      default:
> >> >> >        return error("Invalid record");
> >> >>
> >> >> What warning is this silencing?
> >>
> >> BitcodeReader.cpp(5543): warning C4065: switch statement contains
> >> 'default' but no 'case' labels.
> >>
> >> >> It seems like the correct approach is
> >> >> to remove the switch entirely.
> >> >
> >> >
> >> > +1 - this looks like it should just be:
> >> >
> >> > if (Stream.readRecord(...))
> >> >   return error(...);
> >>
> >> I could do that too, it is currently a placeholder for a TODO when the
> >> full ThinLTO pipeline is hooked up. The comment just above is:
> >>
> >>   // TODO: Read a record. This interface will be completed when ThinLTO
> >>   // importing is added so that it can be tested.
> >>
> >> Eventually it will parse FS_CODE_COMBINED_ENTRY records. If there is a
> >> strong preference to remove this as Aaron suggested, I can do that
> >> though.
> >
> >
> > *shrug* not too fussed (& my suggestion's wrong too, of course, I guess
> it
> > should be "Stream.readRecord(Entry.ID, Record); return error("Invalid
> > record");" - no if/condition/control flow/etc), but probably wouldn't
> hurt
> > to just strip out the placeholder code for now and leave the FIXME to
> > describe where this code is going.
> >
> > *looks at the code some more*
> > So this function is called currently, but always fails? Is that
> > tested/useful? (I'm sure it was all reviewed by people with far more
> context
> > than me when it went in, so don't let me derail you or anything - I'm
> just a
> > tad confused at first glance)
>
> It is currently reached from an interface in lib/Object that is not
> yet used, to support lazy parsing of function summary data. I have
> some code out for review in D13515 that uses the greedy parsing
> interface for tests via llvm-link, but I didn't flesh this one out
> completely since I didn't add a call yet for the lazy parsing (I
> expect there to be tradeoffs and I'm not yet sure which approach will
> win overall). So that's why I sketched out most of the support in this
> routine but I didn't fill in the record handling yet. I'd prefer to
> leave most of this in since I expect to add calls to it as well. Let
> me know if that seems reasonable though!
>

I'll defer to the judgment of your original code reviewers (which is to
say, it was reviewed/signed off, so I'm not going to suggest changing that
now if this was all clear in the original review)

Just for myself, I'm a bit more hard-line about not committing
untested/unexercised code (because it makes it hard to tell that it's
properly tested - if it's not tested in the original commit it's not easy
to tell that it needs to be considered in the test matrix when the code
does become live (usually I only look at the new code to ensure /that/ is
tested, rather than that it lights up a whole bunch of dormant/untested
code in the process))


>
> Thanks,
> Teresa
>
> >
> >>
> >>
> >> Teresa
> >>
> >> >
> >> >>
> >> >>
> >> >> ~Aaron
> >> >>
> >> >> >    }
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > llvm-commits mailing list
> >> >> > llvm-commits at lists.llvm.org
> >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> >> _______________________________________________
> >> >> llvm-commits mailing list
> >> >> llvm-commits at lists.llvm.org
> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> Teresa Johnson | Software Engineer | tejohnson at google.com |
> 408-460-2413
> >
> >
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151021/1e0ba33d/attachment.html>