This is an archive of the discontinued LLVM Phabricator instance.

Do not use default arguments of DataLayout::getPointer*. NFC
ClosedPublic

Authored by MaskRay on Nov 14 2017, 11:11 PM.

Event Timeline

MaskRay created this revision.Nov 14 2017, 11:11 PM
bjope added a subscriber: bjope.Nov 25 2017, 1:15 AM
bjope added inline comments.
source/Expression/IRInterpreter.cpp
387–388

I like that you are trying to get rid of the default arguments to those methods, because I think that there are some "errors" lurking around when using the default arguments. And that is probably the reason for the FIXME in DataLayout (it is about going through the code and making sure we pass the _correct_ address space at every call to those methods).

However, when I read your code here it is hard for me to tell if you have analyzed that it is correct to always use address space zero here. It is still a "magic" 0 in the argument.

Could you perhaps write this as:

// Address space is always 0 here because .... 
unsigned AS = 0;
return Malloc(m_target_data.getPointerSize(AS),
              m_target_data.getPointerPrefAlignment(AS));

to give the constant a name, and preferably also include a comment describing why it should be zero.

MaskRay updated this revision to Diff 124514.Nov 27 2017, 9:34 PM

Remove MallocPointer because it turns out it is not used.

MaskRay marked an inline comment as done.Nov 27 2017, 9:35 PM

Friendly ping...

clayborg resigned from this revision.Dec 5 2017, 3:33 PM

I don't believe I am the right guy to review this.

zturner accepted this revision.Dec 5 2017, 3:36 PM

If you're just removing dead code, then lgtm

This revision is now accepted and ready to land.Dec 5 2017, 3:36 PM
This revision was automatically updated to reflect the committed changes.