Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1800773002:
Output the version of MSVS in chrome://version (Closed)
|
Created:
4 years, 9 months ago by Will Harris Modified:
4 years, 9 months ago CC:
chromium-reviews, brucedawson, Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOutput the version of MSVS in chrome://version
BUG=440500
Committed: https://crrev.com/30ff76ae0a9bd4f054e51520d19453420d534abf
Cr-Commit-Position: refs/heads/master@{#381159}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add error #
Total comments: 6
Patch Set 3 : code review changes #Messages
Total messages: 19 (8 generated)
Description was changed from ========== Output the version of MSVS in chrome://version BUG=440500 ========== to ========== Output the version of MSVS in chrome://version BUG=440500 ==========
wfh@chromium.org changed reviewers: + bauerb@chromium.org
WDYT?
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
I like it with one suggestion. https://codereview.chromium.org/1800773002/diff/1/chrome/browser/ui/webui/ver... File chrome/browser/ui/webui/version_ui.cc (right): https://codereview.chromium.org/1800773002/diff/1/chrome/browser/ui/webui/ver... chrome/browser/ui/webui/version_ui.cc:148: html_source->AddString("compiler", "MSVC"); I'd rather have this be #error so that on the next compiler upgrade I will know to add a new entry. That is, please remove the generic MSVC fallback.
at some point once we know 2015 has stuck we should probably remove the 2013 line as well. https://codereview.chromium.org/1800773002/diff/1/chrome/browser/ui/webui/ver... File chrome/browser/ui/webui/version_ui.cc (right): https://codereview.chromium.org/1800773002/diff/1/chrome/browser/ui/webui/ver... chrome/browser/ui/webui/version_ui.cc:148: html_source->AddString("compiler", "MSVC"); On 2016/03/14 21:43:07, brucedawson wrote: > I'd rather have this be #error so that on the next compiler upgrade I will know > to add a new entry. That is, please remove the generic MSVC fallback. Done.
lgtm Related aside - should we have some #if checks somewhere to check the version number of the compiler being used? For instance, Chromium might actually build with VS 2015 RTM but that will just cause problems. It would seem valuable to have an exact minimum version such that people will be stopped with an informative message instead of hitting inscrutable problems. If such a check (should be in base so it is hit early) is appropriate then we could either add it now or when Update 2 ships.
On 2016/03/14 22:00:13, brucedawson wrote: > lgtm > > Related aside - should we have some #if checks somewhere to check the version > number of the compiler being used? For instance, Chromium might actually build > with VS 2015 RTM but that will just cause problems. It would seem valuable to > have an exact minimum version such that people will be stopped with an > informative message instead of hitting inscrutable problems. > > If such a check (should be in base so it is hit early) is appropriate then we > could either add it now or when Update 2 ships. yes that's probably another CL though. :) maybe needs a bug. also we'd need to be careful of breaking things like CEF... owner please take a look.
wfh@chromium.org changed reviewers: + dbeam@chromium.org - bauerb@chromium.org
dbeam reviewed the last change to this line of code so +dbeam for owners thoughts. also +thakis for info.
https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/version_ui.cc (right): https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:143: #if _MSC_VER == 1900 can this #else + #if be #elif and one less #endif? https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:151: #endif nit: is it possible to use mingw/g++ on windows? https://groups.google.com/a/chromium.org/forum/#!topic/chromium-discuss/vagGh... seems to imply "no" if so: #if defined(OS_WIN) #if defined(__clang__) html_source->AddString("compiler", "clang"); #elif defined(_MSC_VER) && _MSC_VER == 1800 html_source->AddString("compiler", "MSVC 2013"); #elif defined(_MSC_VER) && _MSC_VER == 1900 html_source->AddString("compiler", "MSVC 2015"); #else html_source->AddString("compiler", "Unknown"); #endif #endif // defined(OS_WIN) otherwise, lgtm https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:151: #endif please add #endif // defined(OS_WIN) to final #endif
Thanks for the review. https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/version_ui.cc (right): https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:143: #if _MSC_VER == 1900 On 2016/03/15 00:45:12, Dan Beam wrote: > can this #else + #if be #elif and one less #endif? Done. https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:151: #endif On 2016/03/15 00:45:12, Dan Beam wrote: > please add #endif // defined(OS_WIN) to final #endif Done. https://codereview.chromium.org/1800773002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/version_ui.cc:151: #endif On 2016/03/15 00:45:12, Dan Beam wrote: > nit: is it possible to use mingw/g++ on windows? > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-discuss/vagGh... > seems to imply "no" > > if so: > > #if defined(OS_WIN) > #if defined(__clang__) > html_source->AddString("compiler", "clang"); > #elif defined(_MSC_VER) && _MSC_VER == 1800 > html_source->AddString("compiler", "MSVC 2013"); > #elif defined(_MSC_VER) && _MSC_VER == 1900 > html_source->AddString("compiler", "MSVC 2015"); > #else > html_source->AddString("compiler", "Unknown"); > #endif > #endif // defined(OS_WIN) > > otherwise, lgtm pretty sure you can't use g++ except maybe for base? certainly not chrome code... I'd like to still support Bruce's idea of erroring if we forget to update this when we move to VS2018 so I'll leave the #error if it's MSVS but also support the "Unknown" compiler if nothing else matches.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1800773002/#ps40001 (title: "code review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800773002/40001
Message was sent while issue was closed.
Description was changed from ========== Output the version of MSVS in chrome://version BUG=440500 ========== to ========== Output the version of MSVS in chrome://version BUG=440500 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Output the version of MSVS in chrome://version BUG=440500 ========== to ========== Output the version of MSVS in chrome://version BUG=440500 Committed: https://crrev.com/30ff76ae0a9bd4f054e51520d19453420d534abf Cr-Commit-Position: refs/heads/master@{#381159} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/30ff76ae0a9bd4f054e51520d19453420d534abf Cr-Commit-Position: refs/heads/master@{#381159} |
