Add validation to prevent error on empty if block

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.82%. Comparing base (6168843) to head (2588d90).

Additional details and impacted files

[](https://app.codecov.io/gh/vgvassilev/clad/pull/952?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) ```diff @@ Coverage Diff @@ ## master #952 +/- ## ========================================== + Coverage 93.80% 93.82% +0.01% ========================================== Files 55 55 Lines 7831 7832 +1 ========================================== + Hits 7346 7348 +2 + Misses 485 484 -1 ``` | [Files](https://app.codecov.io/gh/vgvassilev/clad/pull/952?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [include/clad/Differentiator/ReverseModeVisitor.h](https://app.codecov.io/gh/vgvassilev/clad/pull/952?src=pr&el=tree&filepath=include%2Fclad%2FDifferentiator%2FReverseModeVisitor.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-aW5jbHVkZS9jbGFkL0RpZmZlcmVudGlhdG9yL1JldmVyc2VNb2RlVmlzaXRvci5o) | `97.87% <100.00%> (+0.02%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/vgvassilev/clad/pull/952/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | [Files](https://app.codecov.io/gh/vgvassilev/clad/pull/952?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [include/clad/Differentiator/ReverseModeVisitor.h](https://app.codecov.io/gh/vgvassilev/clad/pull/952?src=pr&el=tree&filepath=include%2Fclad%2FDifferentiator%2FReverseModeVisitor.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-aW5jbHVkZS9jbGFkL0RpZmZlcmVudGlhdG9yL1JldmVyc2VNb2RlVmlzaXRvci5o) | `97.87% <100.00%> (+0.02%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/vgvassilev/clad/pull/952/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev)

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[rohanjulka19]

Added test case. Actually this issue fixes this case

 if (x > 0);

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[rohanjulka19]

Is this fine like this is working and I don't see any issue in returning null stmt for all the null stmts in general ? VisitNullStmt was already implemented in the header file i just added the clone stmts in StmtDiff

[parth-07]

Is this fine like this is working and I don't see any issue in returning null stmt for all the null stmts in general ? VisitNullStmt was already implemented in the header file i just added the clone stmts in StmtDiff

Yes, this seems good.

[rohanjulka19]

That null stmt fix was giving an issue with the below test case

double fn_null_stmts(double x) {
    ;;;;;;;;;;;;;;;;;
    ;;;;;return x;;;;
    ;;;;;;;;;;;;;;;;;
} // = x

So now in case the then block is empty I just add a empty block in place of that for both forward and reverse pass

[vgvassilev]

That null stmt fix was giving an issue with the below test case

double fn_null_stmts(double x) {
    ;;;;;;;;;;;;;;;;;
    ;;;;;return x;;;;
    ;;;;;;;;;;;;;;;;;
} // = x

So now in case the then block is empty I just add a empty block in place of that for both forward and reverse pass

@gojakuch could probably comment on the intent of this test.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[parth-07]

Is this fine like this is working and I don't see any issue in returning null stmt for all the null stmts in general ? VisitNullStmt was already implemented in the header file i just added the clone stmts in StmtDiff

It does not seem like the pull-request solution is based on this idea. Why did you diverge from this?

[rohanjulka19]

double fn_null_stmts(double x) {
    ;;;;;;;;;;;;;;;;;
    ;;;;;return x;;;;
    ;;;;;;;;;;;;;;;;;
} // = x

If we follow that idea for this test case it considers each semicolon as a statement so we get a bunch of lines with semicolons. If you want I can just revert to older idea and push it, this test case can be ignored as it still works for this, just gives a very long result

[gojakuch]

That null stmt fix was giving an issue with the below test case

double fn_null_stmts(double x) {
    ;;;;;;;;;;;;;;;;;
    ;;;;;return x;;;;
    ;;;;;;;;;;;;;;;;;
} // = x

So now in case the then block is empty I just add a empty block in place of that for both forward and reverse pass

@gojakuch could probably comment on the intent of this test.

right, when I implemented the NullStmt support it seemed an ugly idea to copy all the meaningless ;; into the derivative code, so it doesn't. this test is there to test an arbitrary number of NullStmt repetitions that should be removed.

[gojakuch]

So now in case the then block is empty I just add a empty block in place of that for both forward and reverse pass

idk that seems better in my opinion because, in general, copying a bunch of nullstmts into the produced code where they are placed on a new line each does sound a bit awkward and makes the code less readable (which is what that test case tries to check). however, since the behaviour of the produced code doesn't change I don't think it's a big deal. but (if I understood this correctly) I think creating if (cond) {} instead of an if (cond) ; is kinda a fair trade-off for not creating

;
;

out of ;; or even more semicolons (a case that one could worsen infinitely by adding more and more ; as opposed to the if statement that you only add an empty block to once). but I don't claim this to be the only right opinion and it depends on the priorities of Clad.

[parth-07]

If we follow that idea for this test case it considers each semicolon as a statement so we get a bunch of lines with semicolons.

I think that is fine. We are staying true to the primal code. I definitely prefer a general solution that reduces code branches over a restricted solution that requires more checks. Also, note that if condition is not the only place that can have NullStmt as the body. In a more restrictive solution, we will need more checks in other areas as well, such as loops.

If you want I can just revert to older idea and push it, this test case can be ignored as it still works for this, just gives a very long result

Yes, I would prefer that. @vgvassilev What do you think about this?

[gojakuch]

I think that is fine. We are staying true to the primal code. I definitely prefer a general solution that reduces code branches over a restricted solution that requires more checks. Also, note that if condition is not the only place that can have NullStmt as the body. In a more restrictive solution, we will need more checks in other areas as well, such as loops.

@parth-07 ok I see your point. that makes sense as well. maybe let's just make a pair of check-execs for this nullstmt test case, so that it at least tests the unchanged behaviour of visiting a bunch of NullStmts, and not check the produced code then.

[rohanjulka19]

can we just change the test case to have one or two semicolons it will yield the same outcome anyways

[gojakuch]

can we just change the test case to have one or two semicolons it will yield the same outcome anyways

yeah, whatever works out for you then. make it two semicolons. just keep some sort of a meaningful check for this, in case someone touches the nullstmt again later.

[vgvassilev]

If we follow that idea for this test case it considers each semicolon as a statement so we get a bunch of lines with semicolons.

I think that is fine. We are staying true to the primal code. I definitely prefer a general solution that reduces code branches over a restricted solution that requires more checks. Also, note that if condition is not the only place that can have NullStmt as the body. In a more restrictive solution, we will need more checks in other areas as well, such as loops.

If you want I can just revert to older idea and push it, this test case can be ignored as it still works for this, just gives a very long result

Yes, I would prefer that. @vgvassilev What do you think about this?

I do not have a strong opinion. This test case is rather artificial and I do not expect to have such cases in real code. If we go with what @parth-07 suggest we can adjust that test to use a little less semicolons ;)

[rohanjulka19]

ok since now we are handling null stmts we do not need to check for empty if blocks in the VisitIfStmt code so removed it and now using 2 semicolons for the above test

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[parth-07]

Please update the commit message to reflect the final solution.