Page MenuHomePhabricator

[clang-format] Adds a FormatStyleSet
ClosedPublic

Authored by krasimir on Dec 21 2017, 5:30 AM.

Details

Summary

This patch adds a FormatStyleSet for storing per-language FormatStyles for the
purposes of formatting code blocks inside the main code.

Diff Detail

Repository
rC Clang

Event Timeline

krasimir created this revision.Dec 21 2017, 5:30 AM
klimek added inline comments.Dec 21 2017, 7:06 AM
lib/Format/Format.cpp
893–894

LanguageFound can't be true here.

899–903

This seems a bit backwards. I'd have expected us to build up a Style structure after parsing above, and then just copy that into the result in the end.

krasimir added inline comments.Dec 21 2017, 7:12 AM
lib/Format/Format.cpp
899–903

Do you mean to build up a StyleSet structure?

krasimir marked an inline comment as done.Dec 21 2017, 7:44 AM
krasimir updated this revision to Diff 127887.Dec 21 2017, 7:45 AM
  • Address review comments
krasimir updated this revision to Diff 127888.Dec 21 2017, 7:46 AM
  • Remove accidentally created file
krasimir updated this revision to Diff 128743.Jan 5 2018, 6:00 AM
  • Address review comments
krasimir marked 2 inline comments as done.Jan 5 2018, 6:00 AM
klimek added inline comments.Jan 5 2018, 7:07 AM
lib/Format/Format.cpp
918–919

Optional: I'd probably slightly re-structure the above to:

if (!LanguageFound) {
  if (Styles.empty() || Styles[0].Language != FS::LK_None) {
    return make_error_code(PE::Unsuitable);
  }
  ...
}
934

Should we rather check that a style that's added doesn't have its own Styles map? (I'd say you're not allowed to add a Style that was gotten into another StyleSet).

krasimir updated this revision to Diff 128921.Jan 8 2018, 6:40 AM
  • Address review comments
krasimir marked 2 inline comments as done.Jan 8 2018, 6:40 AM
bkramer added a subscriber: bkramer.Jan 8 2018, 7:41 AM
bkramer added inline comments.
include/clang/Format/Format.h
1700

Why unique_ptr?

1706

Document what happens if the Style for this language is in the set already.

lib/Format/Format.cpp
916

std::move

939

You can std::move Style here, it's passed by value.

939

Can Style.Language ever be LK_None? Does that even make sense?

krasimir updated this revision to Diff 128932.Jan 8 2018, 8:01 AM
krasimir marked 5 inline comments as done.
  • Address review comments
krasimir updated this revision to Diff 128933.Jan 8 2018, 8:02 AM
  • Remove accidentally added file
krasimir edited reviewers, added: bkramer; removed: klimek, djasper.Jan 8 2018, 8:54 AM
krasimir added subscribers: djasper, klimek.
This revision is now accepted and ready to land.Jan 9 2018, 2:49 AM
This revision was automatically updated to reflect the committed changes.