This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Reordering parameters in getFile and getFileOrSTDIN
ClosedPublic

Authored by abhina.sreeskantharajan on Mar 23 2021, 5:43 AM.

Details

Summary

In future patches I will be setting the IsText parameter frequently so I will refactor the args to be in the following order. I have removed the FileSize parameter because it is never used.

 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getFile(const Twine &Filename, bool IsText = false,
         bool RequiresNullTerminator = true, bool IsVolatile = false);

 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getFileOrSTDIN(const Twine &Filename, bool IsText = false,
                bool RequiresNullTerminator = true);

static ErrorOr<std::unique_ptr<MB>>
getFileAux(const Twine &Filename, uint64_t MapSize, uint64_t Offset,
           bool IsText, bool RequiresNullTerminator, bool IsVolatile);

 static ErrorOr<std::unique_ptr<WritableMemoryBuffer>>
 getFile(const Twine &Filename, bool IsVolatile = false);

Diff Detail

Event Timeline

abhina.sreeskantharajan requested review of this revision.Mar 23 2021, 5:43 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMar 23 2021, 5:43 AM
abhina.sreeskantharajan edited the summary of this revision. (Show Details)

Move FileSize to the end because it is never used.

abhina.sreeskantharajan edited the summary of this revision. (Show Details)

After going through all the function usages, I realized FileSize is not used anywhere. I can remove this parameter.

Overall, this seems reasonable to me. I have a slight concern that it might cause surprising failures for downstream users, that aren't picked up at build time (due to implicit conversions), but I don't think you need to worry too much about that.

llvm/include/llvm/Support/MemoryBuffer.h
78–80

You need to remove the reference to FileSize from the description here.

llvm/lib/Support/MemoryBuffer.cpp
108–109

Are there users of the FileSize here? If not, can you repeat the change for this function?

abhina.sreeskantharajan edited the summary of this revision. (Show Details)

I've removed FileSize from getFileAux and WritableMemoryBuffer::getFile as well.

abhina.sreeskantharajan marked 2 inline comments as done.Mar 24 2021, 5:46 AM
abhina.sreeskantharajan added inline comments.
llvm/include/llvm/Support/MemoryBuffer.h
78–80

Thanks for catching that, I updated the comment.

llvm/lib/Support/MemoryBuffer.cpp
108–109

No, I was able to remove FileSize from here as well as WritableMemoryBuffer::getFile too. Thanks

abhina.sreeskantharajan marked 2 inline comments as done.Mar 24 2021, 5:46 AM
jhenderson accepted this revision.Mar 25 2021, 1:42 AM

LGTM, with nit.

llvm/lib/Support/MemoryBuffer.cpp
281

Whilst you're modifying, add the named parameter comment for the 0, so that it's not just a magic number.

This revision is now accepted and ready to land.Mar 25 2021, 1:42 AM

Rebase and fix formatting.

abhina.sreeskantharajan marked an inline comment as done.Mar 25 2021, 5:16 AM
abhina.sreeskantharajan added inline comments.
llvm/lib/Support/MemoryBuffer.cpp
281

Thanks for catching, this is fixed now.

abhina.sreeskantharajan marked an inline comment as done.Mar 25 2021, 5:16 AM
This revision was landed with ongoing or failed builds.Mar 25 2021, 6:48 AM
This revision was automatically updated to reflect the committed changes.