- I would suggest to avoid "continue" within a loop so that the code is simpler to understand, at least for me.
I personally find the code easier to read with the continue.
Hmmm. I had to read the code to check it, and I did it twice. The point is that there is 3 exit points instead of 1 in the block, which is not in itself very bad, but:
for(...) { if (ccc) xxx; ... foo++; }
It looks like "foo++" is always executed, and you have to notice that xxx a few lines above is a continue to realise that it is only when ccc is false. This is also disconcerting if it happens to be the "rare" case, i.e. here most of the time the char is not '%', so most of the time foo is not incremented, although it is a the highest level. Also the code with continue does not really put forward that what is counted is '%' followed by a non '%'. Note that the corresponding execution code (pgplsql/src/pl_exec.c) uses one continue to get rid of the second '%', which is quite defendable in that case as it is a kind of exception, but the main condition remains a simple if block. Final argument, the structured version is shorter than the unstructured version, with just the two continues removed, and one negation also removed.
to also turn the ereport()s into elog()s since the user should never see them.
I'm not sure why elog is better than ereport in that case: ISTM that it is an error worth reporting if it ever happens, say there is another syntax added later on which is not caught for some reason by the compile-time check, so I would not change it.
one note: this patch can enforce a compatibility issues - a partially broken functions, where some badly written RAISE statements was executed newer.
I am not against this patch, but it should be in extra check probably ?? Or we have to documented it as potential compatibility issue
