singhsrb abandoned this revision.
singhsrb added a comment.
Thanks everyone for their inputs!
I think @simpkins suggestions are definitely better than the main idea of
this commit (which is to move out code from the loop). So, I am abandoning this
commit. If we simplify this function, I
singhsrb added a comment.
@simpkins I definitely prefer
int i;
for (i = 0; i < argc; ++i) {
if (strcmp(argv[i], "--") == 0) {
break;
} else if (strcmp("-d", argv[i]) == 0 ||
strcmp("--daemon", argv[i]) == 0) {
simpkins added inline comments.
INLINE COMMENTS
> chg.c:368-397
> enum {
> SERVE = 1,
> DAEMON = 2,
> SERVEDAEMON = SERVE | DAEMON,
> TIME = 4,
> };
> unsigned int state = 0;
I agree with Yuya that the original code is
singhsrb added a comment.
I just refactored the code a little more to take care of redundant loop
execution check when `argc == 0`. I am guessing you mean you don't like the
code in terms of readability because at runtime, this commit is strictly at
least equal or better than the earlier
singhsrb updated this revision to Diff 2506.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D943?vs=2468=2506
REVISION DETAIL
https://phab.mercurial-scm.org/D943
AFFECTED FILES
contrib/chg/chg.c
CHANGE DETAILS
diff --git a/contrib/chg/chg.c
yuja added a comment.
What I don't like is this change increases complexity of the loop condition,
checking argc at two places, and incrementing i at two places, conditionally.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D943
To: singhsrb, #hg-reviewers,
singhsrb added a comment.
In my opinion, just checking for `hg -R ... serve` would not solve the
problem. If we go through that route, we would probably want to always detect
the serve command rather than mostly detecting the serve command. However, if
we do go in that direction and it
quark added a comment.
I think the general rule of extracting one-time condition to outside a loop
is correct. But the `i == 0` check may be subject to change - if we do want to
support `hg -R ... serve` for example, `i == 0` may be changed to `iscmdname`
or something. And this diff would
singhsrb added a comment.
Just to be clear: this change was also for readability apart from
performance improvement. I personally prefer conditional code inside the loops
to not keep checking if its the first or last iteration because then one can
say that `the loop checks for conditions
singhsrb updated this revision to Diff 2468.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D943?vs=2441=2468
REVISION DETAIL
https://phab.mercurial-scm.org/D943
AFFECTED FILES
contrib/chg/chg.c
CHANGE DETAILS
diff --git a/contrib/chg/chg.c
yuja added subscribers: quark, yuja.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.
I prefer the original code since it looks simpler and there wouldn't be
measurable performance win.
@quark which do you like?
INLINE
singhsrb created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
The loop needlessly checks the condition after the first iteration.
Therefore, moving this condition outside the loop. Compiler can potentially
optimize this
12 matches
Mail list logo
