[llvm] r229953 - IR: Extract macros from DILocation, NFC

[David Blaikie]

On Fri, Feb 20, 2015 at 10:51 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Feb-19, at 21:51, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Thu, Feb 19, 2015 at 6:28 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > Author: dexonsmith
> > Date: Thu Feb 19 20:28:49 2015
> > New Revision: 229953
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=229953&view=rev
> > Log:
> > IR: Extract macros from DILocation, NFC
> >
> > `DILocation` is a lightweight wrapper.  Its accessors check for null and
> > the correct type, and then forward to `MDLocation`.
> >
> > Extract a couple of macros to do the `dyn_cast_or_null<>`
> >
> > Any evidence that cast_or_null would be insufficient? (this would only
> occur if we had code actually querying the fields of DILocation that
> referred to some other metadata node than an MDLocation? That seems like a
> clear bug - but I could believe we could have such things, I just want to
> check that that's the case before we settle for code that would allow such
> bugs if we could avoid having the bar that low at all)
>
> The bar is *already* set that low.


I know it's set that low in the sense that it's not enforced to be any
stronger - but what I meant was: are we actually violating this (does the
bar need to be that low today). If, today, we never trigger the assert if
we use cast_or_null, I'd be inclined to use that now rather than waiting
until later.

Does it exhibit in existing test cases? (otherwise I wouldn't mind having a
test case to demonstrate the problem)


>   I plan to raise it, but
> I'm trying to minimize semantic changes until the new
> hierarchy is in place.
>
> In particular, these should be `cast<>`s (`cast_or_null<>` is
> fair too friendly).  Any place that asserts seems like a
> clear bug to me (in the exceedingly rare (and possibly non-
> existent) case that the caller wants the 0/nullptr/"" value,
> the caller should do this check itself).
>
> >
> > and default
> > return logic.  I'll be using these to minimize error-prone boilerplate
> > when I move the new hierarchy into place -- since all the other
> > subclasses of `DIDescriptor` will similarly become lightweight wrappers.
> >
> > (Note that I hope to obsolete these wrappers fairly quickly, with the
> > goal of renaming the underlying types (e.g., I'll rename `MDLocation` to
> > `DILocation` once the name is free).)
> >
> > Modified:
> >     llvm/trunk/include/llvm/IR/DebugInfo.h
> >
> > Modified: llvm/trunk/include/llvm/IR/DebugInfo.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=229953&r1=229952&r2=229953&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)
> > +++ llvm/trunk/include/llvm/IR/DebugInfo.h Thu Feb 19 20:28:49 2015
> > @@ -252,6 +252,19 @@ public:
> >    void replaceAllUsesWith(MDNode *D);
> >  };
> >
> > +#define RETURN_FROM_RAW(VALID, DEFAULT)
>         \
> > +  do {
>        \
> > +    if (auto *N = getRaw())
>         \
> > +      return VALID;
>         \
> > +    return DEFAULT;
>         \
> > +  } while (false)
> > +#define RETURN_DESCRIPTOR_FROM_RAW(DESC, VALID)
>         \
> > +  do {
>        \
> > +    if (auto *N = getRaw())
>         \
> > +      return DESC(dyn_cast_or_null<MDNode>(VALID));
>         \
> > +    return DESC(static_cast<const MDNode *>(nullptr));
>        \
> > +  } while (false)
> > +
> >  /// \brief This is used to represent ranges, for array bounds.
> >  class DISubrange : public DIDescriptor {
> >    friend class DIDescriptor;
> > @@ -933,25 +946,13 @@ class DILocation : public DIDescriptor {
> >  public:
> >    explicit DILocation(const MDNode *N) : DIDescriptor(N) {}
> >
> > -  unsigned getLineNumber() const {
> > -    if (auto *L = getRaw())
> > -      return L->getLine();
> > -    return 0;
> > -  }
> > -  unsigned getColumnNumber() const {
> > -    if (auto *L = getRaw())
> > -      return L->getColumn();
> > -    return 0;
> > -  }
> > +  unsigned getLineNumber() const { RETURN_FROM_RAW(N->getLine(), 0); }
> > +  unsigned getColumnNumber() const { RETURN_FROM_RAW(N->getColumn(),
> 0); }
> >    DIScope getScope() const {
> > -    if (auto *L = getRaw())
> > -      return DIScope(dyn_cast_or_null<MDNode>(L->getScope()));
> > -    return DIScope(nullptr);
> > +    RETURN_DESCRIPTOR_FROM_RAW(DIScope, N->getScope());
> >    }
> >    DILocation getOrigLocation() const {
> > -    if (auto *L = getRaw())
> > -      return DILocation(dyn_cast_or_null<MDNode>(L->getInlinedAt()));
> > -    return DILocation(nullptr);
> > +    RETURN_DESCRIPTOR_FROM_RAW(DILocation, N->getInlinedAt());
> >    }
> >    StringRef getFilename() const { return getScope().getFilename(); }
> >    StringRef getDirectory() const { return getScope().getDirectory(); }
> > @@ -1042,6 +1043,9 @@ public:
> >    bool Verify() const;
> >  };
> >
> > +#undef RETURN_FROM_RAW
> > +#undef RETURN_DESCRIPTOR_FROM_RAW
> > +
> >  /// \brief Find subprogram that is enclosing this scope.
> >  DISubprogram getDISubprogram(const MDNode *Scope);
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150220/40ee0e16/attachment.html>