This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Remove duplciate code from Invalid BOM detection
ClosedPublic

Authored by MyDeveloperDay on Oct 12 2019, 9:34 AM.

Details

Summary

Review comments on D68767: [clang-format] NFC - Move functionality into functions to help code structure asked that this duplicated code in clang-format was moved to one central location that being SourceManager (where it had originally be copied from I assume)

Moved function into static function ContentCache::getInvalidBOM(...) - (closest class to where it was defined before)
Updated clang-format to call this static function

Added unit tests for said new function in BasicTests

Sorry not my normal code area so may have the wrong reviewers. (but your names were on the recent history)

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Oct 12 2019, 9:34 AM

Thank you for taking care of this! When I fixed a bug in this part of the code last time, I had to do it in both files.

owenpan added inline comments.Oct 12 2019, 5:35 PM
clang/include/clang/Basic/SourceManager.h
232

Can you make the parameter const?

clang/lib/Basic/SourceManager.cpp
98

Add const to the parameter to be more precise?

MyDeveloperDay marked 2 inline comments as done.

Updating with review comments

klimek added inline comments.Oct 14 2019, 1:50 AM
clang/include/clang/Basic/SourceManager.h
232

StringRef is inherently const though, right? I think the added const is confusing.

I tend to agree mainly given that StringRef is used without const elsewhere. I'll bow to better judgement.

owenpan added inline comments.Oct 15 2019, 12:36 AM
clang/include/clang/Basic/SourceManager.h
232

Oops! I meant const StringRef &:

static const char *getInvalidBOM(const StringRef &BufStr);

StringRef already is a const pointer type, so the idiomatic way is to pass it by value.

I did a quick search of the LLVM code base and found dozens of const StringRef &, including [[ https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/Twine.h#L524 | the Twine API ]].

I did a quick search of the LLVM code base and found dozens of const StringRef &, including [[ https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/Twine.h#L524 | the Twine API ]].

Twine is "special", don't be influenced by that. Most of the others are probably mistakes. It's idiomatic to pass StringRef by value.

Thanks for clearing it up!

MyDeveloperDay marked 2 inline comments as done.

I think we agree it should be (StringRef ), no const,no reference

owenpan accepted this revision.Oct 23 2019, 1:31 AM

LGTM

This revision is now accepted and ready to land.Oct 23 2019, 1:31 AM
This revision was automatically updated to reflect the committed changes.