[#928] Impl producing compiler error when required return is missing#958
[#928] Impl producing compiler error when required return is missing#958evgTSV wants to merge 2 commits intoForNeVeR:mainfrom
Conversation
… optimize codegen for constan values for if/else, while/do..while. Enhance do..while codegen. Improve the ConstantEvaluator. Add tests.
kant2002
left a comment
kant2002
left a comment
There was a problem hiding this comment.
Overall I see that this PR can be split into separate things
- Error/warnigns handling which is not present
- Improved control flow analysis
I really-really like to have improved control flow analysis in separate PR, since error reporting does not care if it right or wrong. It just report error/warnings based on what component say it.
Error/warnings behavior is completely separate subject, and we can happiely fight tooth and nails about default value for a flag, while technical PR can be speed run.
In general my recommendations, if you need to refactor something to achieve you goal, separate refactor and actual functionality. that's easier to review, and speedrun each part. Sometimes refactorign alone easier to review, and I can just approve it without too much thinking.
| BasicBlocks.Add(currentBlock); | ||
| } | ||
| BasicBlock nextBlock = new(); | ||
| BasicBlock newBlock = new(); |
There was a problem hiding this comment.
Why these changes? can you undo renaming. If you want refactoring, and names cleanup, please in separate PR.
| var newBlock = new BasicBlock(); | ||
| currentBlock.Targets.Add(newBlock); | ||
| currentBlock = newBlock; | ||
| bool alwaysTaken = |
There was a problem hiding this comment.
This part should happens during lowering of If/While/do instead of in the flow graph. When you have constant value in the conditionalGoto that mean instead of creation of ConditionalGoto we can immidiately create Goto or Nop. No need to create ConditionalGoto and then discard it during BB generation. that's only complicate logic and make CGoto more complex then needed.
| return ($"Expression {expression} cannot be evaluated as constant expression.", null); | ||
| } | ||
| } | ||
| catch (Exception e) |
There was a problem hiding this comment.
We don't use exceptions in compilers, if something blow-up that's correctness error. It should never happens.
| // Do nothing here. | ||
| } | ||
|
|
||
| int missing_return() |
There was a problem hiding this comment.
That's default behaviour for most C compilers, no matter how much @ForNeVeR want to be strict about C23 (which I don't consult yet on this part) compatibility is more improtant even to marketing efforts. How can I say - "Just compile you old MUD" or something silly like that if by default it would be rejected.
There was a problem hiding this comment.
Hmmm.. actually, now, it is undefined behaviour, so compiler may generate compile time errors in this case.
There was a problem hiding this comment.
It obviously may generate errors but that increase chances that people get tired configure our compiler. I very much worry about that
There was a problem hiding this comment.
It obviously may generate errors
Is there an error for code like:
if (true) return;?
|
For the record, I am okay with minor refactoring and actual work in same PR, but at least in separate commits please. |
4 participants
Closes #928
Right now the compiler can handle missing returns. Moreover, it can evaluate the value of a conditional statement (if it's constant) to determine whether it's necessary or redundant in order to drop unreachable paths. As a result, codegen has become simpler.
Besides, I've reworked a sequence of statements in the do..while loop. This allowed for easier optimization of the generated code, since the statements are now in the proper order.
Also, there is a TODO: I assume we should evaluate NULL literals in the future.