Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1323693002:
Subzero: Provide a macro for iterating over instruction variables. (Closed)
|
Created:
5 years, 3 months ago by John Modified:
5 years, 3 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSubzero: Provide a macro for iterating over instruction variables.
This makes it easier and less error-prone to implement the relatively common
pattern of looking at all the Variable operands contained within an instruction.
BUG= none
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=ec3f56532be1792d04ed470221df663bb8ca9c19
Patch Set 1 #
Total comments: 27
Patch Set 2 : Addresses comments. #Patch Set 3 : Caches the current operand. #
Total comments: 8
Patch Set 4 : Addresses comments. #
Total comments: 9
Patch Set 5 : "Addresses comments" #
Messages
Total messages: 17 (2 generated)
stichnot@chromium.org changed reviewers: + stichnot@chromium.org
https://codereview.chromium.org/1323693002/diff/1/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1323693002/diff/1/src/IceCfg.cpp#newcode590 src/IceCfg.cpp:590: const bool IsDest = false; constexpr https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode1 src/IceInstVarIter.h:1: //===- subzero/src/IceInstVarIter.h - Iterate over inst vars-----*- C++ -*-===// s/vars-/vars / https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode23 src/IceInstVarIter.h:23: /// Variables in an Instruction. This lead to the following pattern being leads or led https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode73 src/IceInstVarIter.h:73: /// The hariest, scariest, most confusing parts here are Init2 and Cond2, so hairiest https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode87 src/IceInstVarIter.h:87: /// FOREACH_VAR_IN_INST is), and initialize it with nullptr. Why nullptr? initializes https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:103: /// But there is one more thing we need to do before jumping to the iterators iterator's https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:106: /// make weird line breaks https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:115: /// Subzero, assuming this is always true is dangerous and could lead to It is in fact safe to assume that in Subzero. But the compiler cannot know that, and so changing the second && operand to be true probably makes the code more efficient. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:126: /// We use Var -- which should be a valid C++ identifier -- to uniquefy names -- I think uniquify is the more common spelling of this made-up word. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:130: for (SizeT ___I##Var##___ = 0, ___##Var##Index##___ = 0, \ I looked more closely at C++ reserved identifiers, and it seems we should not be using identifiers with a double underscore anywhere, nor identifiers starting with an underscore plus an uppercase letter. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:143: #define IndexOfVarInInst(Var) ((const SizeT)___##Var##Index##___) I wonder if we could find good names for the three macros that are more unifying, like share a common prefix or something?
jpp@chromium.org changed reviewers: + ascull@google.com, jvoung@chromium.org
Subzero: Provide a macro for iterating over instruction variables. This makes it easier and less error-prone to implement the relatively common pattern of looking at all the Variable operands contained within an instruction. This work is heavily based on Jim's effort on this matter.
https://codereview.chromium.org/1323693002/diff/1/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1323693002/diff/1/src/IceCfg.cpp#newcode590 src/IceCfg.cpp:590: const bool IsDest = false; On 2015/08/29 23:20:39, stichnot wrote: > constexpr Do you mind static constexpr ?? https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode1 src/IceInstVarIter.h:1: //===- subzero/src/IceInstVarIter.h - Iterate over inst vars-----*- C++ -*-===// On 2015/08/29 23:20:39, stichnot wrote: > s/vars-/vars / Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode23 src/IceInstVarIter.h:23: /// Variables in an Instruction. This lead to the following pattern being On 2015/08/29 23:20:39, stichnot wrote: > leads or led Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode73 src/IceInstVarIter.h:73: /// The hariest, scariest, most confusing parts here are Init2 and Cond2, so On 2015/08/29 23:20:39, stichnot wrote: > hairiest Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode87 src/IceInstVarIter.h:87: /// FOREACH_VAR_IN_INST is), and initialize it with nullptr. Why nullptr? On 2015/08/29 23:20:39, stichnot wrote: > initializes Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:103: /// But there is one more thing we need to do before jumping to the iterators On 2015/08/29 23:20:39, stichnot wrote: > iterator's Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:106: /// make On 2015/08/29 23:20:39, stichnot wrote: > weird line breaks Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:115: /// Subzero, assuming this is always true is dangerous and could lead to On 2015/08/29 23:20:39, stichnot wrote: > It is in fact safe to assume that in Subzero. > > But the compiler cannot know that, and so changing the second && operand to be > true probably makes the code more efficient. Acknowledged. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:126: /// We use Var -- which should be a valid C++ identifier -- to uniquefy names -- On 2015/08/29 23:20:39, stichnot wrote: > I think uniquify is the more common spelling of this made-up word. Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:130: for (SizeT ___I##Var##___ = 0, ___##Var##Index##___ = 0, \ On 2015/08/29 23:20:39, stichnot wrote: > I looked more closely at C++ reserved identifiers, and it seems we should not be > using identifiers with a double underscore anywhere, nor identifiers starting > with an underscore plus an uppercase letter. Done. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:143: #define IndexOfVarInInst(Var) ((const SizeT)___##Var##Index##___) On 2015/08/29 23:20:39, stichnot wrote: > I wonder if we could find good names for the three macros that are more > unifying, like share a common prefix or something? I am not a huge fan of prefixes/suffixes as I find the resulting names less "natural" to read. I personally like these names, but I agree that the two accessor macros are misleading as there's nothing preventing people from writing IndexOfVarInInst(Var) I rewrote this code so that IndexOfVar and IndexOfVarOperand now expand to #define IndexOfVarInInst(Var) \ OnlyValidIn_FOREACH_VAR_IN_INSTS_Body(_Sz_##Var##Index_) #define IndexOfVarOperandInInst(Var) \ OnlyValidIn_FOREACH_VAR_IN_INSTS_Body(_Sz_I##Var##_) With this implementation, we get a "nice" error src/IceInstVarIter.h:142:31: note: expanded from macro 'IndexOfVarInInst' #define IndexOfVarInInst(Var) IsOnlyValidInFOREACH_VAR_IN_INSTS(Var##Index) thoughts?
https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode38 src/IceInstVarIter.h:38: /// definition is ugly, awful, painful-to-read, using it is fairly simple: Why no use a function for this and pass in a lambda. That would make the definition easier to read and be "more C++". Surely the compiler can do some optimization to avoid having to do function calls?
https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcode38 src/IceInstVarIter.h:38: /// definition is ugly, awful, painful-to-read, using it is fairly simple: On 2015/08/31 16:30:21, ascull wrote: > Why no use a function for this and pass in a lambda. That would make the > definition easier to read and be "more C++". Surely the compiler can do some > optimization to avoid having to do function calls? I am all for avoiding macros/macro-trickery and other things (though it may not seem so with the assembler tests and now this), but in this case macros are the right tools. * I don't think C++ compilers would be smart enough to inline functions taking lambdas / std::function. Those aren't function pointers, but closures -- they have other state the needs to be passed around. * Lambdas are also weird-looking entities ([<captures>](<parameters) {...}) * Sometimes you might need access to "internal" loop state (e.g., control variable, variable index.) If we use lambdas, we need to pass those as parameters to **every** lambda -- even the ones that do no use them. * The clumsy implementation is hidden behind a library, but the exposed interface is somewhat natural: FOREACH_VAR_IN_INST(Var, Instr) { ... } versus foreacg_var_in_inst(Instr, [...](SizeT op_index, SizeT VarIndex, SizeT VarIndexInInstr, SizeT VarIndexInOperand) { ... }); * Jim tried using an inline function with an std::function parameter, which led to a small performance regression.
https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:130: for (SizeT ___I##Var##___ = 0, ___##Var##Index##___ = 0, \ On 2015/08/31 16:26:39, John wrote: > On 2015/08/29 23:20:39, stichnot wrote: > > I looked more closely at C++ reserved identifiers, and it seems we should not > be > > using identifiers with a double underscore anywhere, nor identifiers starting > > with an underscore plus an uppercase letter. > > Done. Not quite. You're not allowed to start the identifier with "_Sz" because of an underscore followed by an uppercase letter. https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:143: #define IndexOfVarInInst(Var) ((const SizeT)___##Var##Index##___) On 2015/08/31 16:26:39, John wrote: > On 2015/08/29 23:20:39, stichnot wrote: > > I wonder if we could find good names for the three macros that are more > > unifying, like share a common prefix or something? > > I am not a huge fan of prefixes/suffixes as I find the resulting names less > "natural" to read. I personally like these names, but I agree that the two > accessor macros are misleading as there's nothing preventing people from writing > > IndexOfVarInInst(Var) > > I rewrote this code so that IndexOfVar and IndexOfVarOperand now expand to > > #define IndexOfVarInInst(Var) \ > OnlyValidIn_FOREACH_VAR_IN_INSTS_Body(_Sz_##Var##Index_) > > #define IndexOfVarOperandInInst(Var) \ > OnlyValidIn_FOREACH_VAR_IN_INSTS_Body(_Sz_I##Var##_) > > With this implementation, we get a "nice" error > > src/IceInstVarIter.h:142:31: note: expanded from macro 'IndexOfVarInInst' > #define IndexOfVarInInst(Var) IsOnlyValidInFOREACH_VAR_IN_INSTS(Var##Index) > > thoughts? Sick. :) https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:94: /// here to avoid crazy code like "avoid" - did you mean "allow"? https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:102: /// 4) Init2 is where the voodoo starts. It declares a Variable * local The multi-line indentation for 4) and 5) and 6) is different from 1-3. https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:129: /// which is not quite right. Cond2 would evalute to false if evaluate https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:132: /// problems formatting
https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/1/src/IceInstVarIter.h#newcod... src/IceInstVarIter.h:130: for (SizeT ___I##Var##___ = 0, ___##Var##Index##___ = 0, \ On 2015/08/31 18:22:21, stichnot wrote: > On 2015/08/31 16:26:39, John wrote: > > On 2015/08/29 23:20:39, stichnot wrote: > > > I looked more closely at C++ reserved identifiers, and it seems we should > not > > be > > > using identifiers with a double underscore anywhere, nor identifiers > starting > > > with an underscore plus an uppercase letter. > > > > Done. > > Not quite. You're not allowed to start the identifier with "_Sz" because of an > underscore followed by an uppercase letter. http://en.cppreference.com/w/cpp/language/identifiers has all the rules. Underscore + lowercase is good, so it ending in an underscore.
https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp File src/IceRegAlloc.cpp (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { Looking at how this is used I was a bit confused as I expected Var to already exist as a variable. The Linux kernel has some macro'd for loops (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) and you have to pass in the variable to use which solves the problem but leaves the variable in the outer scope. But you still have the identifier name to used for uniqueness. I can't think how you can do it with: FOREACH_VAR_IN_INST(Variable *Var, Inst) I get get stuck with '*Var' and can't work out how to strip the *.
https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:94: /// here to avoid crazy code like On 2015/08/31 18:22:21, stichnot wrote: > "avoid" - did you mean "allow"? Done. https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:102: /// 4) Init2 is where the voodoo starts. It declares a Variable * local On 2015/08/31 18:22:21, stichnot wrote: > The multi-line indentation for 4) and 5) and 6) is different from 1-3. Done. https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:129: /// which is not quite right. Cond2 would evalute to false if On 2015/08/31 18:22:21, stichnot wrote: > evaluate Done. https://codereview.chromium.org/1323693002/diff/40001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:132: /// problems On 2015/08/31 18:22:21, stichnot wrote: > formatting Done. https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp File src/IceRegAlloc.cpp (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { On 2015/08/31 19:03:17, ascull wrote: > Looking at how this is used I was a bit confused as I expected Var to already The header file declaring the macros states that Var needs to be an undefined id, but self documenting code would be better. > exist as a variable. The Linux kernel has some macro'd for loops > (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) and you > have to pass in the variable to use which solves the problem but leaves the > variable in the outer scope. But you still have the identifier name to used for > uniqueness. > > I can't think how you can do it with: > > FOREACH_VAR_IN_INST(Variable *Var, Inst) This looks redundant -- Var is always a Variable *. Besides, how do you get rid of 'Variable *'? > > I get get stuck with '*Var' and can't work out how to strip the *.
https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp File src/IceRegAlloc.cpp (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { On 2015/08/31 19:11:05, John wrote: > On 2015/08/31 19:03:17, ascull wrote: > > Looking at how this is used I was a bit confused as I expected Var to already > The header file declaring the macros states that Var needs to be an undefined > id, but self documenting code would be better. > > > exist as a variable. The Linux kernel has some macro'd for loops > > (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) and > you > > have to pass in the variable to use which solves the problem but leaves the > > variable in the outer scope. But you still have the identifier name to used > for > > uniqueness. > > > > I can't think how you can do it with: > > > > FOREACH_VAR_IN_INST(Variable *Var, Inst) > This looks redundant -- Var is always a Variable *. It's not redundant as it shows the declaration of the variable instead of it magically appearing. Being a C-style language comes with the expectation of there being a clear declaration for variables. > Besides, how do you get rid of 'Variable *'? As I've said both above and below, I'm not sure if this will work or how to do it. I can get rid of 'Variable ' but not the *. I was just suggesting it as it feels more similar to the native for each and you might have known some preprocessor trick to get it to work. > > > > I get get stuck with '*Var' and can't work out how to strip the *.
I agree it would be nice to have a pony and a way to declare Variable *Var in the macro invocation, but if not, LGTM. https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:100: /// to be legal. We also want to avoid warnings about "dangling else"s. "from being legal" ? https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:142: /// We use Var -- which should be a valid C++ identifier -- to uniquify names I think this last paragraph should be de-indented one space, because it's not part of step 6), right?
https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h File src/IceInstVarIter.h (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:100: /// to be legal. We also want to avoid warnings about "dangling else"s. On 2015/08/31 21:31:11, stichnot wrote: > "from being legal" ? Done. https://codereview.chromium.org/1323693002/diff/60001/src/IceInstVarIter.h#ne... src/IceInstVarIter.h:142: /// We use Var -- which should be a valid C++ identifier -- to uniquify names On 2015/08/31 21:31:11, stichnot wrote: > I think this last paragraph should be de-indented one space, because it's not > part of step 6), right? Done. https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp File src/IceRegAlloc.cpp (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { On 2015/08/31 19:28:05, ascull wrote: > On 2015/08/31 19:11:05, John wrote: > > On 2015/08/31 19:03:17, ascull wrote: > > > Looking at how this is used I was a bit confused as I expected Var to > already > > The header file declaring the macros states that Var needs to be an undefined > > id, but self documenting code would be better. > > > > > exist as a variable. The Linux kernel has some macro'd for loops > > > (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) and > > you > > > have to pass in the variable to use which solves the problem but leaves the > > > variable in the outer scope. But you still have the identifier name to used > > for > > > uniqueness. > > > > > > I can't think how you can do it with: > > > > > > FOREACH_VAR_IN_INST(Variable *Var, Inst) > > This looks redundant -- Var is always a Variable *. > > It's not redundant as it shows the declaration of the variable instead of it > magically appearing. Being a C-style language comes with the expectation of > there being a clear declaration for variables. > It is redundant -- Types in variable declarations are always redundant when you initialize said variable -- the compiler knows what type the variable is so re-stating it is redundant. I did not say "wrong", "ugly", or "poor style." :) > > Besides, how do you get rid of 'Variable *'? > > As I've said both above and below, I'm not sure if this will work or how to do > it. I can get rid of 'Variable ' but not the *. I was just suggesting it as it > feels more similar to the native for each and you might have known some > preprocessor trick to get it to work. > Oh, mistyped. I wanted to know how you could get rid of the 'Variable ' (not the *) > > > > > > I get get stuck with '*Var' and can't work out how to strip the *. >
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as ec3f56532be1792d04ed470221df663bb8ca9c19 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp File src/IceRegAlloc.cpp (right): https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { On 2015/08/31 22:07:03, John wrote: > On 2015/08/31 19:28:05, ascull wrote: > > On 2015/08/31 19:11:05, John wrote: > > > On 2015/08/31 19:03:17, ascull wrote: > > > > Looking at how this is used I was a bit confused as I expected Var to > > already > > > The header file declaring the macros states that Var needs to be an > undefined > > > id, but self documenting code would be better. > > > > > > > exist as a variable. The Linux kernel has some macro'd for loops > > > > (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) > and > > > you > > > > have to pass in the variable to use which solves the problem but leaves > the > > > > variable in the outer scope. But you still have the identifier name to > used > > > for > > > > uniqueness. > > > > > > > > I can't think how you can do it with: > > > > > > > > FOREACH_VAR_IN_INST(Variable *Var, Inst) > > > This looks redundant -- Var is always a Variable *. > > > > It's not redundant as it shows the declaration of the variable instead of it > > magically appearing. Being a C-style language comes with the expectation of > > there being a clear declaration for variables. > > > It is redundant -- Types in variable declarations are always redundant when you > initialize said variable -- the compiler knows what type the variable is so > re-stating it is redundant. I did not say "wrong", "ugly", or "poor style." :) > > > > Besides, how do you get rid of 'Variable *'? > > > > As I've said both above and below, I'm not sure if this will work or how to do > > it. I can get rid of 'Variable ' but not the *. I was just suggesting it as it > > feels more similar to the native for each and you might have known some > > preprocessor trick to get it to work. > > > > Oh, mistyped. I wanted to know how you could get rid of the 'Variable ' (not the > *) > > > > > > > > > I get get stuck with '*Var' and can't work out how to strip the *. > > > #define remove_variable(decl) abc##decl #define abc_Variable /* Note: this is empty */ `remove_variable(Variable *Var)` -> `abc_Variable *Var` -> `*Var` The limitation is that it requires you to know all the possibilities ahead of time.
Message was sent while issue was closed.
On 2015/08/31 22:31:56, ascull wrote: > https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp > File src/IceRegAlloc.cpp (right): > > https://codereview.chromium.org/1323693002/diff/60001/src/IceRegAlloc.cpp#new... > src/IceRegAlloc.cpp:184: FOREACH_VAR_IN_INST(Var, Inst) { > On 2015/08/31 22:07:03, John wrote: > > On 2015/08/31 19:28:05, ascull wrote: > > > On 2015/08/31 19:11:05, John wrote: > > > > On 2015/08/31 19:03:17, ascull wrote: > > > > > Looking at how this is used I was a bit confused as I expected Var to > > > already > > > > The header file declaring the macros states that Var needs to be an > > undefined > > > > id, but self documenting code would be better. > > > > > > > > > exist as a variable. The Linux kernel has some macro'd for loops > > > > > (https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) > > and > > > > you > > > > > have to pass in the variable to use which solves the problem but leaves > > the > > > > > variable in the outer scope. But you still have the identifier name to > > used > > > > for > > > > > uniqueness. > > > > > > > > > > I can't think how you can do it with: > > > > > > > > > > FOREACH_VAR_IN_INST(Variable *Var, Inst) > > > > This looks redundant -- Var is always a Variable *. > > > > > > It's not redundant as it shows the declaration of the variable instead of it > > > magically appearing. Being a C-style language comes with the expectation of > > > there being a clear declaration for variables. > > > > > It is redundant -- Types in variable declarations are always redundant when > you > > initialize said variable -- the compiler knows what type the variable is so > > re-stating it is redundant. I did not say "wrong", "ugly", or "poor style." :) > > > > > > Besides, how do you get rid of 'Variable *'? > > > > > > As I've said both above and below, I'm not sure if this will work or how to > do > > > it. I can get rid of 'Variable ' but not the *. I was just suggesting it as > it > > > feels more similar to the native for each and you might have known some > > > preprocessor trick to get it to work. > > > > > > > Oh, mistyped. I wanted to know how you could get rid of the 'Variable ' (not > the > > *) > > > > > > > > > > > > I get get stuck with '*Var' and can't work out how to strip the *. > > > > > > > #define remove_variable(decl) abc##decl > #define abc_Variable /* Note: this is empty */ > > `remove_variable(Variable *Var)` -> `abc_Variable *Var` -> `*Var` > > The limitation is that it requires you to know all the possibilities ahead of > time. I know some neat macro tricks, but this one deserves the crown! :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
