Fixed
Status Update
No update yet.
Comments
th...@chromium.org
libstdc++ enforces the same constraint. We discovered this over at https://bugs.chromium.org/p/chromium/issues/detail?id=947527#c4
c#6 on that bug says that the libc++ behavior is intended. +ericwf@ to confirm if the behavior in this bug is correct too
c#6 on that bug says that the libc++ behavior is intended. +ericwf@ to confirm if the behavior in this bug is correct too
th...@chromium.org
As a larger point, I'd expect that `use_custom_libcxx = false` will bitrot. It shouldn't be on chrome devs to keep that config compiling. If you care about it, I think you should fix issues yourself rather than filing bugs. (Or you can file a bug on yourself.) (...at least once we've had a stable release with libc++.)
The 2nd sentence inhttps://bugs.chromium.org/p/chromium/issues/detail?id=947527#c6 sounds like rejecting vector<const string> is what's intended, which contradicts the first sentence :)
The 2nd sentence in
br...@chromium.org
I'm 99% sure that the behavior being reported by VC++'s STL is indeed a bug and that std::vector<const T> is indeed illegal. I know we had this discussion when we switched to VC++ 2017, which first added this enforcement (or some other version around that time frame).
br...@chromium.org
> I'd expect that `use_custom_libcxx = false` will bitrot.
Sure, but this isn't just a charming quirk of the VC++ STL. The code is wrong (not in a horrible or obvious way, but still ill-formed) and I think passing that back to the author is appropriate. Since libstdc++ also enforces this it seems possible that libc++ will eventually, and either way I think we prefer to follow the standard.
Sure, but this isn't just a charming quirk of the VC++ STL. The code is wrong (not in a horrible or obvious way, but still ill-formed) and I think passing that back to the author is appropriate. Since libstdc++ also enforces this it seems possible that libc++ will eventually, and either way I think we prefer to follow the standard.
th...@chromium.org
Re-reading the first paragraph of https://crbug.com/chromium/961424#c2 I hasten to add: Filing bugs on making libc++ as strict as Microsoft's stl or libstdc++ is of course very welcome!
th...@chromium.org
Looks like we collided.
I disagree we should give authors work for stuff that makes it past the cq. I think a better sequence is:
- File bug on toolchain folks to let libc++ catch problem
- Roll libc++ with fix and as part of that clean up the codebase
That way, we get the same effect, but only people who are familiar with toolchain arcana are exposed to it.
I disagree we should give authors work for stuff that makes it past the cq. I think a better sequence is:
- File bug on toolchain folks to let libc++ catch problem
- Roll libc++ with fix and as part of that clean up the codebase
That way, we get the same effect, but only people who are familiar with toolchain arcana are exposed to it.
bu...@chops-service-accounts.iam.gserviceaccount.com
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/a0abf76fef534a606c5b66502682f524349a49d1
commit a0abf76fef534a606c5b66502682f524349a49d1
Author: Ilia Samsonov <isamsonov@google.com>
Date: Sat May 11 00:41:10 2019
Checking for gtest name duplication.
Fail TestLauncher if disabled tests shares a name with another test.
Added unit tests for TestLauncher disabled tests logic.
Changed TestResultTracker unit test to better reflect disabled test tags.
Changed problematic disabled tests name that caused CQ to fail.
DISABLED_BrowserCloseInfiniteBeforeUnload was removed since
it is a copy of BrowserCloseInfiniteBeforeUnload.
Bug: 961424,960619
Change-Id: I064a6d5c06e5dfccab2e54d2d5e54ff3903e81f1
Reviewed-on:https://chromium-review.googlesource.com/c/chromium/src/+/1603483
Commit-Queue: Ilia Samsonov <isamsonov@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658846}
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/base/test/launcher/test_launcher.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/base/test/launcher/test_launcher.h
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/base/test/launcher/test_launcher_unittest.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/base/test/launcher/test_results_tracker_unittest.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/chrome/browser/extensions/api/system_indicator/system_indicator_manager.h
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/chrome/browser/unload_browsertest.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
[modify]https://crrev.com/a0abf76fef534a606c5b66502682f524349a49d1/ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc
commit a0abf76fef534a606c5b66502682f524349a49d1
Author: Ilia Samsonov <isamsonov@google.com>
Date: Sat May 11 00:41:10 2019
Checking for gtest name duplication.
Fail TestLauncher if disabled tests shares a name with another test.
Added unit tests for TestLauncher disabled tests logic.
Changed TestResultTracker unit test to better reflect disabled test tags.
Changed problematic disabled tests name that caused CQ to fail.
DISABLED_BrowserCloseInfiniteBeforeUnload was removed since
it is a copy of BrowserCloseInfiniteBeforeUnload.
Bug: 961424,960619
Change-Id: I064a6d5c06e5dfccab2e54d2d5e54ff3903e81f1
Reviewed-on:
Commit-Queue: Ilia Samsonov <isamsonov@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658846}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
is...@google.com
This issue was migrated from crbug.com/chromium/961424?no_tracker_redirect=1
[Monorail components added to Component Tags custom field.]
[Monorail components added to Component Tags custom field.]
Description
"The C++ Standard forbids containers of const elements because allocator<const T> is ill-formed."
An std::vector<const std::string> was added in
C:\src\chromium3\src\third_party\depot_tools\win_toolchain\vs_files\818a152b3f1da991c1725d85be19a0f27af6bab4\VC\Tools\MSVC\14.16.27023\include\xmemory0(943,2): error: static_assert failed due to requirement '!is_const_v<const std::basic_string<char, std::char_traits<char>, std::allocator<char> > >' "The C++ Standard forbids containers of const elements because allocator<const T> is ill-formed."
static_assert(!is_const_v<_Ty>,
^ ~~~~~~~~~~~~~~~~
...
void SetUpExpectCalls(std::vector<const std::string> test_names) {
Not the most serious bug ever, probably, but being able to turn off custom_libcxx is handy for A/B testing, and I assume that this check is correct, so we are currently non-conformant.
To repro just build base_unittests with use_custom_libcxx = false.
thomasanderson@ - is there any way to get this check added to libcxx? It's a simple static_assert, although I imagine that there are quite a few layers of bureaucracy to get such a change made. Do you know where to file a bug to request this?
I don't go looking for these things, honestly...