[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

George Rimar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 14 04:10:58 PDT 2020


grimar added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:16079
+  auto Style9 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true);
+  ASSERT_TRUE((bool)Style9);
 }
----------------
It looks like it is very similar to "Test 7:" and can be a part of it?
E.g:


```
...
llvm::consumeError(Style7a.takeError());

// <comment>
auto Style7b = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true);
ASSERT_TRUE((bool)Style8);
```


================
Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:39
+
+TEST(ObjectYAML, UnkownOption) {
+  std::string InputYAML = "Binary: AAAA\n"
----------------
Unk**n**ownOption


================
Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:40
+TEST(ObjectYAML, UnkownOption) {
+  std::string InputYAML = "Binary: AAAA\n"
+                          "InvalidKey: InvalidValue";
----------------
`std::string`->`StringRef`?


================
Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:41
+  std::string InputYAML = "Binary: AAAA\n"
+                          "InvalidKey: InvalidValue";
+  BinaryHolder BH;
----------------
Will this work if we switch the order of `InvalidKey` and `Binary`?

```
  std::string InputYAML = "InvalidKey: InvalidValue\n"
                          "Binary: AAAA";
```

I expect that yes and I think it is reasonable to do it to show that we don't stop parsing an YAML
when there is an unknown key.


================
Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:44
+  llvm::yaml::Input Input(InputYAML);
+  // test 1: default in trying to parse invalid key is an error case
+  Input >> BH;
----------------
No full stop at the end.


================
Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:57
+  EXPECT_EQ(BH2.Binary, BH_GT.Binary);
+  EXPECT_EQ(Input2.error().value(), 0);
+}
----------------
Do you need `BH_GT`? Can it be just:

```
  // test 2: only warn about invalid key if actively set.
  llvm::yaml::Input Input2(InputYAML);
  BinaryHolder BH2;
  Input2.setAllowUnknownKeys(true);
  Input2 >> BH2;
  EXPECT_EQ(Input2.error().value(), 0);
  EXPECT_EQ(BH2.Binary, yaml::BinaryRef("AAAA"));
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86137/new/

https://reviews.llvm.org/D86137



More information about the cfe-commits mailing list