rewrite_to_chrome_style generates conflicting edits |
|||||
Issue descriptionrewrite_to_chrome_style generates in some (limited) cases conflicting edits for the same piece of file. Let's use this bug to understand these cases and to apply fixes as needed.
,
Dec 28 2016
It looks like many of these are things that are inconsistently detected as constants in templates: - TraceTraits.h - WebGLImageConversions.cpp - ASCIIFastPath.h
,
Dec 30 2016
It turns out that fooBar -> kFooBar vs foo_bar is caused by the fact that the same location matches more than once as a VarDecl. In most cases, IsProbablyConst returns true, but in one of the cases initializer->isEvaluatable(context) is false.
,
Dec 30 2016
I noticed it's often in templates; is it returning false when the template isn't fully instantiated, and is used as part of another template?
,
Dec 30 2016
RE: #c4 Possibly. I don't really know :-( What would be the consequence of confirming this? Would this translate into changes of RewriteToChromeStyle.cpp that would eliminate inconsistent renaming decisions? Also - I wonder if some form of memoization would help here - we could maintain a map from clang::SourceLocation to new_name to avoid inconsistencies when the same *decl* location is processed within a single invocation of rewrite_to_chrome_style. - There is some small potential for perf improvements from this, but this potential seems very small :-) - Memoization would only span one compilation unit - it wouldn't help with inconsistencies generated in header files when included from different compilation units. I am not sure if the inconsistencies above are of that nature.
,
Jan 4 2017
,
Jan 6 2017
How did we get the list of conflicting edits for this listed above? The list could show the .cpp files where they came from? Then it would be easier to repro and fix
,
Jan 6 2017
Conflicting edit: third_party/WebKit/Source/wtf/RefVector.h at offset 1307, length 6: "GetVector" != "Vector" This one is different from what you're talking about above I think - it's a vector accessor() returning a Vector. "vector" is in the list https://cs.chromium.org/chromium/src/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp?rcl=0&l=477 to have a Get prefix, but in some case that prefix is not being applied there.
,
Jan 6 2017
RE: #c7 The list in #c1 is written to stderr by tools/clang/scripts/apply_edits.py. At that point we don't know where the edits originated from (i.e. which files was being "compiled" by the tool when it generated the edits). The problem with Get prefix seems to be fixed in the most recent run of the tool I've tried. This problem was indeed expected to be fixed by https://codereview.chromium.org/2601513005. There is no known solution to the problem of inconsistent kConstant vs constant name - see #c2-5
,
Jan 6 2017
,
Jan 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9a1485ee7f50c3980f8055d8f64ea4d30220720 commit b9a1485ee7f50c3980f8055d8f64ea4d30220720 Author: danakj <danakj@chromium.org> Date: Mon Jan 09 16:29:33 2017 Get more consistent decisions if a statement is constant in templates The tool was saying some template-dependent values were both const and not const because when the template parameters meet some criteria it collapses them out of the AST. If we always check the entire subtree of expressions to verify they are evaluatable we get more consistent results. Also don't try to turn this_type_of_name into kThisTypeOfName, as the author explicitly wrote it that way and we end up with the ever fun kThis_type_of_name instead. R=dcheng@chromium.org BUG= 677285 Review-Url: https://codereview.chromium.org/2616213003 Cr-Commit-Position: refs/heads/master@{#442262} [modify] https://crrev.com/b9a1485ee7f50c3980f8055d8f64ea4d30220720/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp [modify] https://crrev.com/b9a1485ee7f50c3980f8055d8f64ea4d30220720/tools/clang/rewrite_to_chrome_style/tests/constants-expected.cc [modify] https://crrev.com/b9a1485ee7f50c3980f8055d8f64ea4d30220720/tools/clang/rewrite_to_chrome_style/tests/constants-original.cc [modify] https://crrev.com/b9a1485ee7f50c3980f8055d8f64ea4d30220720/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc [modify] https://crrev.com/b9a1485ee7f50c3980f8055d8f64ea4d30220720/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
,
Jan 9 2017
Here are the remaining conflicts on an android build: Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp at offset 5476, length 8: "flag_char" != "kFlagChar" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp at offset 5430, length 8: "flag_char" != "kFlagChar" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp at offset 5407, length 8: "flag_char" != "kFlagChar" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26897, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26882, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26859, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26843, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26706, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26692, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26356, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26255, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29805, length 17: "kSelectionWrapper2" != "selection_wrapper2" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29775, length 17: "kSelectionWrapper1" != "selection_wrapper1" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29738, length 17: "kSelectionWrapper2" != "selection_wrapper2" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29710, length 17: "kSelectionWrapper1" != "selection_wrapper1" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29674, length 17: "kSelectionWrapper2" != "selection_wrapper2" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29647, length 17: "kSelectionWrapper1" != "selection_wrapper1" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29609, length 17: "kSelectionWrapper2" != "selection_wrapper2" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29580, length 17: "kSelectionWrapper1" != "selection_wrapper1" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29539, length 17: "kSelectionWrapper2" != "selection_wrapper2" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 29465, length 17: "kSelectionWrapper1" != "selection_wrapper1" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 28970, length 21: "has_trailing_whitespace" != "kHasTrailingWhitespace" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisibleSelection.cpp at offset 28887, length 21: "has_trailing_whitespace" != "kHasTrailingWhitespace" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3844, length 16: "kUpstreamPosition" != "upstream_position" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3780, length 18: "downstream_position" != "kDownstreamPosition" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3716, length 16: "kUpstreamPosition" != "upstream_position" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3696, length 18: "downstream_position" != "kDownstreamPosition" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3616, length 16: "kUpstreamPosition" != "upstream_position" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3450, length 18: "downstream_position" != "kDownstreamPosition" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/core/editing/VisiblePosition.cpp at offset 3304, length 18: "downstream_position" != "kDownstreamPosition"
,
Jan 10 2017
Remaining errors after https://codereview.chromium.org/2622003002/# Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26897, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26882, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26859, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26843, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26706, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26692, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26356, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26255, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp at offset 91159, length 11: "kTrivialPack" != "trivial_pack" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp at offset 90690, length 13: "kTrivialUnpack" != "trivial_unpack" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp at offset 89815, length 11: "kTrivialPack" != "trivial_pack" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp at offset 89759, length 13: "kTrivialUnpack" != "trivial_unpack"
,
Jan 10 2017
Which means I regressed WebGLImageConversion.cpp which means I didn't test it well, woops. Onward and upward.
,
Jan 11 2017
https://codereview.chromium.org/2624253002/ fixes WebGLImageConversion.cpp
,
Jan 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e2c34d46083dc28796e3e68e34b2b8231a8c5cc commit 5e2c34d46083dc28796e3e68e34b2b8231a8c5cc Author: danakj <danakj@chromium.org> Date: Wed Jan 11 17:07:45 2017 rewrite_to_chrome_style: Make is-const decisions more consistent This fixes cases where a templated method accesses a member on its class object, and cases where a function argument is templated and used as the input to a const variable. R=dcheng@chromium.org BUG= 677285 Review-Url: https://codereview.chromium.org/2622003002 Cr-Commit-Position: refs/heads/master@{#442936} [modify] https://crrev.com/5e2c34d46083dc28796e3e68e34b2b8231a8c5cc/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp [modify] https://crrev.com/5e2c34d46083dc28796e3e68e34b2b8231a8c5cc/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc [modify] https://crrev.com/5e2c34d46083dc28796e3e68e34b2b8231a8c5cc/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
,
Jan 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed3c975463926a72676db3793bdbed76926086b0 commit ed3c975463926a72676db3793bdbed76926086b0 Author: danakj <danakj@chromium.org> Date: Wed Jan 11 17:14:00 2017 rewrite_to_chrome_style: Make const decisions aware of const variables When a const variable is initialized by another variable, we recurse to see if that variable is compile-time-const to decide if the former variable is. BUG= 677285 Review-Url: https://codereview.chromium.org/2624253002 Cr-Commit-Position: refs/heads/master@{#442938} [modify] https://crrev.com/ed3c975463926a72676db3793bdbed76926086b0/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp [modify] https://crrev.com/ed3c975463926a72676db3793bdbed76926086b0/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc [modify] https://crrev.com/ed3c975463926a72676db3793bdbed76926086b0/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
,
Jan 11 2017
These still remain: Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26897, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26882, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26859, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26843, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26706, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26692, length 9: "kKeyIsWeak" != "key_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26356, length 11: "kValueIsWeak" != "value_is_weak" Conflicting edit: /usr/local/google/home/danakj/s/c/src/third_party/WebKit/Source/platform/heap/TraceTraits.h at offset 26255, length 9: "kKeyIsWeak" != "key_is_weak" Applied 882828 edits (8 errors) to 8805 files [100.00%]
,
Jan 11 2017
I wish I knew which cpp file hmm
,
Jan 11 2017
Ok third_party/WebKit/Source/modules/indexeddb/IDBObserver.cpp produces these collisions from the one .cpp file.
,
Jan 11 2017
BinaryOperator 0xc146118 '_Bool' '==' |-ImplicitCastExpr 0xc1460e8 'int' <IntegralCast> | `-ImplicitCastExpr 0xc1460d0 'enum WTF::WeakHandlingFlag' <LValueToRValue> | `-DeclRefExpr 0xc145c78 'const enum WTF::WeakHandlingFlag' lvalue Var 0x88ab0f8 'weakHandlingFlag' 'const enum WTF::WeakHandlingFlag' `-ImplicitCastExpr 0xc146100 'int' <IntegralCast> `-DeclRefExpr 0x7fcc590 'enum WTF::WeakHandlingFlag' EnumConstant 0x6efa5d0 'WeakHandlingInCollections' 'enum WTF::WeakHandlingFlag' Result is const: 1 vs CXXOperatorCallExpr 0x7fcc6b8 '<dependent type>' |-UnresolvedLookupExpr 0x7fcc5b8 '<overloaded function type>' lvalue (ADL) = 'operator==' 0x6fe5908 0x6fe6198 0x6fe69e8 0x6fe7108 0x6fe77b8 0x6fe7df8 0x6fe8378 0x7001e48 0x7002598 0x7002c68 0x70032d8 0x7003858 0x649c6e8 0x72b7418 0x72b8608 0x72b97e8 0x72ba9e8 0x72cc468 0x72cd958 0x72cee48 0x72d0358 0x7319438 |-DependentScopeDeclRefExpr 0x7fcc550 '<dependent type>' lvalue `-DeclRefExpr 0x7fcc590 'enum WTF::WeakHandlingFlag' EnumConstant 0x6efa5d0 'WeakHandlingInCollections' 'enum WTF::WeakHandlingFlag' Result is const: 0
,
Jan 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/362c69d27d39af6a9f6945099acb593cd6d96579 commit 362c69d27d39af6a9f6945099acb593cd6d96579 Author: danakj <danakj@chromium.org> Date: Thu Jan 12 01:40:04 2017 rewrite_to_chrome_style: Use constexpr as backdoor to get consistency In the last cases of inconsistent evaluation of compile-time-constants just make them constexpr and then ensure we always see constexpr things as compile-time constants. Actually compiling figures this out even though from the AST we see unresolved things that make this hard. R=dcheng BUG= 677285 Review-Url: https://codereview.chromium.org/2626923004 Cr-Commit-Position: refs/heads/master@{#443115} [modify] https://crrev.com/362c69d27d39af6a9f6945099acb593cd6d96579/third_party/WebKit/Source/platform/heap/TraceTraits.h [modify] https://crrev.com/362c69d27d39af6a9f6945099acb593cd6d96579/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
,
Jan 16 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by lukasza@chromium.org
, Dec 28 2016