New issue
Advanced search Search tips

Issue 643810 link

Starred by 1 user
Status: WontFix
Owner:
lukasza@chromium.org
Closed: Sep 2016
Cc:
danakj@chromium.org
ssid@chromium.org
keishi@chromium.org
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: macro token concatenation strikes again: platform/heap/ThreadState.cpp:1505

Project Member Reported by lukasza@chromium.org, Sep 2 2016 
The tool rewrites |InlineVectorArenaIndex| into |kInlineVectorArenaIndex|, but this obviously doesn't work with token concatenation done by SNAPSHOT_HEAP macro:

--- a/third_party/WebKit/Source/platform/heap/BlinkGC.h
+++ b/third_party/WebKit/Source/platform/heap/BlinkGC.h
...
     enum HeapIndices {
...
-        InlineVectorArenaIndex,
+        kInlineVectorArenaIndex,
...
     };

--- a/third_party/WebKit/Source/platform/heap/ThreadState.cpp
+++ b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
...
#define SNAPSHOT_HEAP(ArenaType)                                                                \
     {                                                                                          \
-        numberOfHeapsReported++;                                                               \
+        number_of_heaps_reported++;                                                               \
         switch (type) {                                                                        \
-        case SnapshotType::HeapSnapshot:                                                       \
-            m_arenas[BlinkGC::ArenaType##ArenaIndex]->takeSnapshot(heapsDumpName + "/" #ArenaType, info);   \
+        case SnapshotType::kHeapSnapshot:                                                       \
+            arenas_[BlinkGC::ArenaType##ArenaIndex]->TakeSnapshot(heaps_dump_name + "/" #ArenaType, info);   \
             break;                                                                             \
-        case SnapshotType::FreelistSnapshot:                                                   \
-            m_arenas[BlinkGC::ArenaType##ArenaIndex]->takeFreelistSnapshot(heapsDumpName + "/" #ArenaType); \
+        case SnapshotType::kFreelistSnapshot:                                                   \
+            arenas_[BlinkGC::ArenaType##ArenaIndex]->TakeFreelistSnapshot(heaps_dump_name + "/" #ArenaType); \
             break;                                                                             \
         default:                                                                               \
             ASSERT_NOT_REACHED();                                                              \


So - we either need to rewrite |InlineVector| to |kInlineVector| in macro invocations like below:
    SNAPSHOT_HEAP(kInlineVector);
Or, we can fix the macro definition to prepend k##ArenaType##ArenaIndex.

This issue feels quite similar to  issue 641033

Comment 1 by danakj@chromium.org, Sep 2 2016 

This reminds me of https://codereview.chromium.org/1676703002

Comment 2 by danakj@chromium.org, Sep 2 2016 

So I'd say change the macro to prepend k##ArenaType##ArenaIndex.

Comment 3 by lukasza@chromium.org, Sep 2 2016

Description: Show this description

Comment 4 by lukasza@chromium.org, Sep 2 2016 

Oh, and another alternative would be to get rid of token concatenation altogether (this would result in a smaller manual CL, because the manual CL wouldn't have to immediately do the InlineVector->kInlineVector rename - this could be postponed until we run the tool [which should manage this rename just fine after there is no token concatenation]).


NoTokenConcatenation-SingleArgOption:

-#define SNAPSHOT_HEAP(ArenaType)                                                                \
+#define SNAPSHOT_HEAP(ArenaTypeArenaIndex)                                                      \
...
- m_arenas[BlinkGC::ArenaType##ArenaIndex]->takeSnapshot(heapsDumpName + "/" #ArenaType, info);   \
+ m_arenas[BlinkGC::ArenaTypeArenaIndex]->takeSnapshot(heapsDumpName + "/" #ArenaTypeArenaIndex, info);   \
...
-    SNAPSHOT_HEAP(InlineVector);
+    SNAPSHOT_HEAP(kInlineVectorArenaIndex);

but this would change the string passed to takeSnapshot / takeFreelistSnapshot method (and I am not sure if such change is okay).


NoTokenConcatenation-TwoArgsOption:

This follows a similar approach to https://codereview.chromium.org/1676703002/patch/80001/90002 where we have some code duplication at the macro invocation sites:

-#define SNAPSHOT_HEAP(ArenaType)                                                                \
+#define SNAPSHOT_HEAP(ArenaTypeArenaIndex, ArenaDescription)                                    \
...
- m_arenas[BlinkGC::ArenaType##ArenaIndex]->takeSnapshot(heapsDumpName + "/" #ArenaType, info);   \
+ m_arenas[BlinkGC::ArenaTypeArenaIndex]->takeSnapshot(heapsDumpName + "/" ArenaDescription, info);   \
...
-    SNAPSHOT_HEAP(InlineVector);
+    SNAPSHOT_HEAP(kInlineVectorArenaIndex, "InlineVector");

Comment 5 by lukasza@chromium.org, Sep 2 2016

Cc: keishi@chromium.org ssid@chromium.org
+ssid@ and keishi@ for opinions (since they've added the token concatenation according to git blame)

Comment 6 by lukasza@chromium.org, Sep 9 2016

Status: WontFix (was: Untriaged)
I guess the easiest thing to do might be to just fold it into a patch to be applied after running the tool (maintained at  https://crbug.com/644384  and https://codereview.chromium.org/2329463004/#ps20001).
►

Sign in to add a comment