[BPF] Avoid repeating MI->getOperand(NumDefs) x3

[tamird]

Remove brace

[tamird]

@eddyz87 could you take a look please? I have some other changes that are based on this that can't build until this lands.

[eddyz87]

In my opinion, whether or not to repeat MI->getOperand(NumDefs) is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.

Also, I don't understand why you landed this change w/o due process.

[tamird]

In D156445#4554232, @eddyz87 wrote:

In my opinion, whether or not to repeat MI->getOperand(NumDefs) is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.

I seems obvious that readability of code-at-HEAD should be of a higher priority than the readability of the commit history; the extant code will be read many more times than the the commit history for the same piece of code.

The description of this change doesn't claim that there is a change in behavior here; is there documentation that requires all changes to have functional changes?

Also, I don't understand why you landed this change w/o due process.

As I understand it, the process is not prescriptive about reviews being required. I see many changes (particularly in this area) that have gone in without review. In this case, I tried to follow "due process", but received no engagement whatsoever. Is filibustering an accepted practice?

[eddyz87]

In D156445#4554360, @tamird wrote:

In D156445#4554232, @eddyz87 wrote:

In my opinion, whether or not to repeat MI->getOperand(NumDefs) is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.

I seems obvious that readability of code-at-HEAD should be of a higher priority than the readability of the commit history; the extant code will be read many more times than the the commit history for the same piece of code.

I disagree that readability of these 2 lines had improved. I see this commit is an unnecessary code shuffling.

The description of this change doesn't claim that there is a change in behavior here; is there documentation that requires all changes to have functional changes?

Also, I don't understand why you landed this change w/o due process.

As I understand it, the process is not prescriptive about reviews being required. I see many changes (particularly in this area) that have gone in without review. In this case, I tried to follow "due process", but received no engagement whatsoever. Is filibustering an accepted practice?

Personally, I wait for reviewers engagements for all my changes.

llvm/lib/Target/BPF/BTFDebug.cpp

 
 #include "BTFDebug.h"
 #include "BPF.h"
 #include "BPFCORE.h"
 #include "MCTargetDesc/BPFMCTargetDesc.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Target/TargetLoweringObjectFile.h"
 #include <optional>
void BTFDebug::beginInstruction(const MachineInstr *MI) {
 
   if (SkipInstruction || MI->isMetaInstruction() ||
       MI->getFlag(MachineInstr::FrameSetup))
     return;
 
   if (MI->isInlineAsm()) {
     // Count the number of register definitions to find the asm string.
     unsigned NumDefs = 0;
-    for (; MI->getOperand(NumDefs).isReg() && MI->getOperand(NumDefs).isDef();
-         ++NumDefs)
-      ;
-
+    while (true) {
+      const MachineOperand &MO = MI->getOperand(NumDefs);
+      if (MO.isReg() && MO.isDef()) {
+        ++NumDefs;
+        continue;
+      }
       // Skip this inline asm instruction if the asmstr is empty.
-    const char *AsmStr = MI->getOperand(NumDefs).getSymbolName();
+      const char *AsmStr = MO.getSymbolName();
       if (AsmStr[0] == 0)
         return;
+      break;
+    }
   }
 
   if (MI->getOpcode() == BPF::LD_imm64) {
     // If the insn is "r2 = LD_imm64 @<an AmaAttr global>",
     // add this insn into the .BTF.ext FieldReloc subsection.
     // Relocation looks like:
     //  . SecName:
     //    . InstOffset