[llvm] r229953 - IR: Extract macros from DILocation, NFC
Duncan P. N. Exon Smith
dexonsmith at apple.com
Fri Feb 20 11:26:00 PST 2015
> On 2015-Feb-20, at 11:16, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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)
It'll be more efficient for me to get the new hierarchy into
place first. Then I can tighten up behaviour of all the
DIDescriptors at once by changing the macro definition. In
particular, this approach requires fewer LTO bootstraps and
fewer runs of `make check` to sort out all the problems.
> 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
> >
More information about the llvm-commits
mailing list