Hi! On Mon, Mar 23, 2020 at 08:16:51PM +0530, kamlesh kumar wrote: > * rtl.h : Defined Tuple for bundling rtx, mode and > unsignedness default as 0
This line is too long (and your mailer wrapped it). No space before colon. Full stop at the end of a line. Write changelogs in the imperative. > Added Extra argument (unsignedp) in emit_library_call and > emit_library_call_value. Use "bool" for boolean arguments (just like anywhere else), not "int". Boolean arguments are problematic usually: there is no way someone can tell from a call site what that argument is, usually. Written as "0" it is even worse than "false". And it doesn't even mean "not unsigned" here, in most cases! It just means "we don't care", or "we don't know", or "whatever". > * testsuite/gcc.target/powerpc/pr88877.c : Newtest That should be in a separate changelog (and be "New." or "New test." or similar). Please split this into a bunch of patches: the first few only change the interface (so add the extra "bool" arguments), but in such a way that no behaviour changes at all. One patch for every function that is used a lot it easiest to review (and to write, and to write the changelog for, this is not extra work for you either, quite the opposite). Followed by a patch that actually changes things. I could not find anywhere where you do not pass "false" for that new bool argument, btw., which illustrates my point here. Do we really want function calls that are indented fifty characters, and then have nine(!) arguments? At some point adding stuff on top makes things topple over, and it is better to restructure things first. Of course that isn't fair to demand of you here, but maybe you see some opportunity to improve things? Segher
