Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784083002:
Fix style for aggregate initilaizer. (Closed)
DescriptionFix style for aggregate initializer.
Appease clang when run with -Wmissing-braces. By rule, the braces can be elided but since std::array is an array wrapped in a struct, it produces a warning.
BUG=706428
Review-Url: https://codereview.chromium.org/2784083002
Cr-Commit-Position: refs/heads/master@{#462291}
Committed: https://chromium.googlesource.com/chromium/src/+/74cea7ee6f641c6f59970a45bc6a5ec334020131
Patch Set 1 #
Messages
Total messages: 21 (9 generated)
skau@chromium.org changed reviewers: + thestig@chromium.org
thestig@: The updated version of clang doesn't like brace elision. PTAL.
thestig@chromium.org changed reviewers: + thakis@chromium.org
I haven't been using uniform initialization syntax, so I'll defer to Nico.
thakis@ Can you take a look? I'm sure this got buried in your queue.
Sorry, I remember not understanding the patch when it arrived, and then it got
lost in my inbox. I still don't understand the change. This seems to work fine:
Nicos-MacBook-Pro:llvm-build thakis$ clang -c test.cc -std=c++11
Nicos-MacBook-Pro:llvm-build thakis$ bin/clang -c test.cc -std=c++11 -isysroot
$(xcrun -show-sdk-path)
Nicos-MacBook-Pro:llvm-build thakis$
~/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang -c test.cc
-std=c++11 -isysroot $(xcrun -show-sdk-path)
Nicos-MacBook-Pro:llvm-build thakis$ cat test.cc
#include <array>
const char kPrinterState[] = "printer-state";
const char kPrinterStateReasons[] = "printer-state-reasons";
const char kPrinterStateMessage[] = "printer-state-message";
constexpr std::array<const char *const, 3> kPrinterAttributes = {
kPrinterState, kPrinterStateReasons, kPrinterStateMessage};
Why is this change needed?
On 2017/04/05 21:41:57, Nico wrote:
> Sorry, I remember not understanding the patch when it arrived, and then it got
> lost in my inbox. I still don't understand the change. This seems to work
fine:
>
> Nicos-MacBook-Pro:llvm-build thakis$ clang -c test.cc -std=c++11
> Nicos-MacBook-Pro:llvm-build thakis$ bin/clang -c test.cc -std=c++11 -isysroot
> $(xcrun -show-sdk-path)
> Nicos-MacBook-Pro:llvm-build thakis$
> ~/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang -c test.cc
> -std=c++11 -isysroot $(xcrun -show-sdk-path)
> Nicos-MacBook-Pro:llvm-build thakis$ cat test.cc
> #include <array>
>
> const char kPrinterState[] = "printer-state";
> const char kPrinterStateReasons[] = "printer-state-reasons";
> const char kPrinterStateMessage[] = "printer-state-message";
>
> constexpr std::array<const char *const, 3> kPrinterAttributes = {
> kPrinterState, kPrinterStateReasons, kPrinterStateMessage};
>
>
> Why is this change needed?
It reports a warning in the simplechrome workflow. Clang reports a warning that
braces are missing. Simple chrome builds include -Wmissing-braces. If the
warnings are superfluous, I could file a bug with the toolchain team to suppress
the warning. It is valid C++ either way.
Description was changed from ========== Fix style for aggregate initilaizer. Clang doesn't recognize brace elision for aggregate initializers so it's been added. Problem surfaced after clang was updated. BUG=706428 ========== to ========== Fix style for aggregate initilaizer. Clang doesn't recognize brace elision for aggregate initializers so it's been added. Problem surfaced after clang was updated. Warning is surfaced when clang is run with -Wmissing-braces BUG=706428 ==========
On 2017/04/05 22:02:14, skau wrote:
> On 2017/04/05 21:41:57, Nico wrote:
> > Sorry, I remember not understanding the patch when it arrived, and then it
got
> > lost in my inbox. I still don't understand the change. This seems to work
> fine:
> >
> > Nicos-MacBook-Pro:llvm-build thakis$ clang -c test.cc -std=c++11
> > Nicos-MacBook-Pro:llvm-build thakis$ bin/clang -c test.cc -std=c++11
-isysroot
> > $(xcrun -show-sdk-path)
> > Nicos-MacBook-Pro:llvm-build thakis$
> > ~/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang -c test.cc
> > -std=c++11 -isysroot $(xcrun -show-sdk-path)
> > Nicos-MacBook-Pro:llvm-build thakis$ cat test.cc
> > #include <array>
> >
> > const char kPrinterState[] = "printer-state";
> > const char kPrinterStateReasons[] = "printer-state-reasons";
> > const char kPrinterStateMessage[] = "printer-state-message";
> >
> > constexpr std::array<const char *const, 3> kPrinterAttributes = {
> > kPrinterState, kPrinterStateReasons, kPrinterStateMessage};
> >
> >
> > Why is this change needed?
>
> It reports a warning in the simplechrome workflow. Clang reports a warning
that
> braces are missing. Simple chrome builds include -Wmissing-braces. If the
> warnings are superfluous, I could file a bug with the toolchain team to
suppress
> the warning. It is valid C++ either way.
Ah, with -Wall it repros. The change lgtm, but the CL description sounds
misguiding. clang does implement brace elision and whatnot (it's only a
warning). What I think is happening: -Wmissing-braces tries to tell you that you
need to match your braces to the struct your initializing so if you have
struct S {
int a;
struct {
int a, b;
} b;
};
you should write `S s = { 4, { 5, 6 } }` and not `S s = { 4, 5, 6 }`. That's
what the warning is designed to catch. Now, std::array<T, n> is morally the same
as
struct array {
T contents[n];
};
and so the warning complains that one set of {} is needed because you're
initializing a struct, and then it wants another to initialize the array in the
struct. So the warning is doing exactly what it's supposed to do.
Description was changed from ========== Fix style for aggregate initilaizer. Clang doesn't recognize brace elision for aggregate initializers so it's been added. Problem surfaced after clang was updated. Warning is surfaced when clang is run with -Wmissing-braces BUG=706428 ========== to ========== Fix style for aggregate initializer. Appease clang when run with -Wmissing-braces. By rule, the braces can be elided but since std::array is an array wrapped in a struct, it produces a warning. BUG=706428 ==========
Thanks. I've updated the comment to something that should be more accurate.
The CQ bit was checked by skau@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by skau@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1491437878808460, "parent_rev":
"ee58c2ec1a33d0c0df5478576b5dc8f26fd2cb4d", "commit_rev":
"74cea7ee6f641c6f59970a45bc6a5ec334020131"}
Message was sent while issue was closed.
Description was changed from ========== Fix style for aggregate initializer. Appease clang when run with -Wmissing-braces. By rule, the braces can be elided but since std::array is an array wrapped in a struct, it produces a warning. BUG=706428 ========== to ========== Fix style for aggregate initializer. Appease clang when run with -Wmissing-braces. By rule, the braces can be elided but since std::array is an array wrapped in a struct, it produces a warning. BUG=706428 Review-Url: https://codereview.chromium.org/2784083002 Cr-Commit-Position: refs/heads/master@{#462291} Committed: https://chromium.googlesource.com/chromium/src/+/74cea7ee6f641c6f59970a45bc6a... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/74cea7ee6f641c6f59970a45bc6a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
