[PATCH] D156136: [BPF] Clean up SelLowering

[Tamir Duberstein via Phabricator via llvm-commits]

tamird added inline comments.


================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:46
+    Val->print(OS);
+    OS << ' ';
+  }
----------------
eddyz87 wrote:
> tamird wrote:
> > eddyz87 wrote:
> > > This prints extra space at the end of the output. I think it should be like this:
> > > 
> > > ```
> > >   if (Val) {
> > >     raw_string_ostream OS(Str);
> > >     OS << ' ';
> > >     Val->print(OS);
> > >   }
> > > ```
> > I don't think so; the caller has passed in a message, which we concat on line 50. We need this space so that the user's message doesn't blend in with ours.
> I mean, I checked it before commenting.
> If you add, say `'|'`, at the beginning and at the end of `OS` you'll get output like this:
> 
> ```
> test3.c:3:10: error: too many arguments|t1: i64 = GlobalAddress<ptr @foo> 0, test3.c:3:10 |
>     3 |   return foo(1, 2, 3, 4, 5, 6, 7);
>       |          ^
> 1 error generated.
> ```
> 
> Note that there is no space after "arguments" and there is an extra space before last `|`. This is because we are doing `Msg.concat(Str)`, not `Str.concat(Msg)`.
> 
> Thanks for addressing the rest of the comments.
Thanks for explaining! Fixed and tightened up the tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156136/new/

https://reviews.llvm.org/D156136