Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674983002:
Add quotes to CR_CLANG_REVISION define (Closed)
DescriptionAdd quotes to CR_CLANG_REVISION define
clang_revision variable may has format, that represents arithmetical
expression. For example – 284979-2.
Althought it is stated that this define should not be used in code,
it is unsafe. It can be made as string, so it will perfectly do it's
job and be much more safe.
R=brettw@chromium.org, dpranke@chromium.org
BUG=
Review-Url: https://codereview.chromium.org/2674983002
Cr-Commit-Position: refs/heads/master@{#448520}
Committed: https://chromium.googlesource.com/chromium/src/+/3331c473d65bc3331cf14e9797b102fdbbf013ea
Patch Set 1 #
Messages
Total messages: 16 (6 generated)
Description was changed from ========== Add quotes to CR_CLANG_REVISION define clang_revision variable may has format, that represents arithmetical expression. For example – 284979-2. Althought it is stated that this define should not be used in code, it is unsafe. It can be made as string, so it will perfectly do it's job and be much more safe. R=brettw@chromium.org, dpranke@chromium.org BUG= ========== to ========== Add quotes to CR_CLANG_REVISION define clang_revision variable may has format, that represents arithmetical expression. For example – 284979-2. Althought it is stated that this define should not be used in code, it is unsafe. It can be made as string, so it will perfectly do it's job and be much more safe. R=brettw@chromium.org, dpranke@chromium.org BUG= ==========
zienag@yandex-team.ru changed reviewers: + scottmg@chromium.org
dpranke@chromium.org changed reviewers: + thakis@chromium.org
It seems like if we're worried about this being actually referenced in files, you'd be better off making it be something that will cause errors, like CR_CLANG_REVISION=error_12346 or #error or someting. thakis@, what would you like here?
On 2017/02/04 17:03:42, Dirk Pranke (slow until Feb 7) wrote: > It seems like if we're worried about this being actually referenced in files, > you'd be better off making it be something that will cause errors, like > CR_CLANG_REVISION=error_12346 or #error or someting. > > thakis@, what would you like here? It hasn't been an issue in practice, so I'd do nothing. But either this cl or what you suggest make sense to me too.
On 2017/02/04 17:23:13, Nico wrote: > On 2017/02/04 17:03:42, Dirk Pranke (slow until Feb 7) wrote: > > It seems like if we're worried about this being actually referenced in files, > > you'd be better off making it be something that will cause errors, like > > CR_CLANG_REVISION=error_12346 or #error or someting. > > > > thakis@, what would you like here? > > It hasn't been an issue in practice, so I'd do nothing. But either this cl or > what you suggest make sense to me too. I'm fine w/ doing nothing, too. I think the CL as it stands doesn't help much.
On 2017/02/04 17:39:28, Dirk Pranke (slow until Feb 7) wrote: > On 2017/02/04 17:23:13, Nico wrote: > > On 2017/02/04 17:03:42, Dirk Pranke (slow until Feb 7) wrote: > > > It seems like if we're worried about this being actually referenced in > files, > > > you'd be better off making it be something that will cause errors, like > > > CR_CLANG_REVISION=error_12346 or #error or someting. > > > > > > thakis@, what would you like here? > > > > It hasn't been an issue in practice, so I'd do nothing. But either this cl or > > what you suggest make sense to me too. > > I'm fine w/ doing nothing, too. I think the CL as it stands doesn't help much. Inspiration for this cl came to me when I tried run clang-tidy (http://clang.llvm.org/extra/clang-tidy/) on chromium (https://chromium.googlesource.com/chromium/src/+/lkgr/docs/clang_tidy.md). There is very useful check – misc-macro-parentheses (http://clang.llvm.org/extra/clang-tidy/checks/misc-macro-parentheses.html), and it's triggers on CR_CLANG_REVISION (suggesting to wrap it in parenthesis) in every file without any possibility to suppress it. And I can't say that it is a false positive, because, strictly speaking, such problem exists. There is of course our "contract", that forbids using of this macro, but for now, there is no way, compiler could understand that there is something wrong if I'll write #if CR_CLANG_REVISION > 284978 or foo = CR_CLANG_REVISIONR * 2;
On 2017/02/06 10:02:49, zienag wrote: > On 2017/02/04 17:39:28, Dirk Pranke (slow until Feb 7) wrote: > > On 2017/02/04 17:23:13, Nico wrote: > > > On 2017/02/04 17:03:42, Dirk Pranke (slow until Feb 7) wrote: > > > > It seems like if we're worried about this being actually referenced in > > files, > > > > you'd be better off making it be something that will cause errors, like > > > > CR_CLANG_REVISION=error_12346 or #error or someting. > > > > > > > > thakis@, what would you like here? > > > > > > It hasn't been an issue in practice, so I'd do nothing. But either this cl > or > > > what you suggest make sense to me too. > > > > I'm fine w/ doing nothing, too. I think the CL as it stands doesn't help much. > > Inspiration for this cl came to me when I tried run clang-tidy > (http://clang.llvm.org/extra/clang-tidy/) on chromium > (https://chromium.googlesource.com/chromium/src/+/lkgr/docs/clang_tidy.md). > There is very useful check – misc-macro-parentheses > (http://clang.llvm.org/extra/clang-tidy/checks/misc-macro-parentheses.html), and > it's triggers on CR_CLANG_REVISION (suggesting to wrap it in parenthesis) in > every file without any possibility to suppress it. And I can't say that it is a > false positive, because, strictly speaking, such problem exists. There is of > course our "contract", that forbids using of this macro, but for now, there is > no way, compiler could understand that there is something wrong if I'll write > > #if CR_CLANG_REVISION > 284978 > > or > > foo = CR_CLANG_REVISIONR * 2; Making clang-tidy be happy seems like a good reason. lgtm if thakis also agrees.
sure, lgtm
The CQ bit was checked by zienag@yandex-team.ru
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": 1486420149841510, "parent_rev":
"3c36d59b4bd011c2bbbf28d59275aa082ede1ff5", "commit_rev":
"3331c473d65bc3331cf14e9797b102fdbbf013ea"}
Message was sent while issue was closed.
Description was changed from ========== Add quotes to CR_CLANG_REVISION define clang_revision variable may has format, that represents arithmetical expression. For example – 284979-2. Althought it is stated that this define should not be used in code, it is unsafe. It can be made as string, so it will perfectly do it's job and be much more safe. R=brettw@chromium.org, dpranke@chromium.org BUG= ========== to ========== Add quotes to CR_CLANG_REVISION define clang_revision variable may has format, that represents arithmetical expression. For example – 284979-2. Althought it is stated that this define should not be used in code, it is unsafe. It can be made as string, so it will perfectly do it's job and be much more safe. R=brettw@chromium.org, dpranke@chromium.org BUG= Review-Url: https://codereview.chromium.org/2674983002 Cr-Commit-Position: refs/heads/master@{#448520} Committed: https://chromium.googlesource.com/chromium/src/+/3331c473d65bc3331cf14e9797b1... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/3331c473d65bc3331cf14e9797b1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
