Minor fixes

[marsupial]

Description

Minor fixes that #836, #838, & #839 depend on that are less intrusive/controversial:

• ASTfunction_call::m_poly can possibly be left uninitialized.
• Duplicate/redefined functions are being silently ignored.
• Preprocessing with boost::wave is currently inserting whitespace into the .osl source.

Tests

Yes

Checklist:

• I have read the contribution guidelines.
• I have previously submitted a Contributor License Agreement.
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[lgritz]

This LGTM! I'll run tests, but will merge if it looks ok to me (I don't feel like I need to wait until next week for this one, it's not a major change).

So the new code still allows function to be declared twice as long as only one has statements? So this operates like a "forward declaration"? Is there any use for this, or should we just disallow the duplicate declaration in the same scope?

[marsupial]

Yes forward declarations are still allowed.
It could easily be disabled, but thought it was a requirement for the language.

I'd vote to keep them:

1. Erring on including 2 headers (or 1 twice) that randomly only declare a function seems harsh.
2. It's currently the only way to do recursion in OSL which may be in use.

Fails:

void recurse(int level) {
    recurse(level);
}

Works:

void recurse(int level);

void recurse(int level) {
    recurse(level);
}

Fixing the recursion issue looks to be non-trivial, but also seems it's a nice feature that it fails unless really explicit about it.

[lgritz]

I'm certainly fine with forward declarations, no reason to disallow them. In fact, we rely on them via stdosl.h to declare most of the built-ins that way. I do like the new property that it's a recognized error for the function to be declared more than once with statements.

Recursion is explicitly disallowed by the OSL spec, and all user functions are inlined. This is (a) for speed, and (b) to eliminate stack overflows or infinite recursion (we try very hard to prevent user errors in shaders from being able to crash a renderer).

[marsupial]

Originally I had thought recursion was disallowed but searching both the spec and google turned up no info. Adding to the pdf in functions or control-flow might be worthwhile.

[lgritz]

Oops, you're right. My oversight. I will fix the spec, I'm not sure how that got lost.

[marsupial]

So 2e2e2fe wound up causing conflicts as it is attempting to fix what d5ad478 did, but from what I can tell 2e2e2fe is inferior:

No filename/sourceline information about redefinition making the problem much more tedious to correct.

It is warning on things that should be legal:

int redclfunc (int f); // ok
int redclfunc (int f); // ok
int redclfunc (int f) { return 0; }  // ok
int redclfunc (int f); // ok

Is currently incorrectly (because fwd decl should be allowed and duped warning) spewing:

test.osl:2: warning: Function 'int redclfunc (int)' declared twice in the same scope
test.osl:3: warning: Function 'int redclfunc (int)' declared twice in the same scope
test.osl:3: warning: Function 'int redclfunc (int)' declared twice in the same scope
test.osl:4: warning: Function 'int redclfunc (int)' declared twice in the same scope
test.osl:4: warning: Function 'int redclfunc (int)' declared twice in the same scope
test.osl:4: warning: Function 'int redclfunc (int)' declared twice in the same scope

Demotion to warning instead of errs can make the compiler spew untruths later:

int redclfunc (int f) { return 0; }  // ok
int redclfunc (int f); // ok
void redclfunc () { int a = 10; }

Leads to this output which is totally incorrect given the source:

test.osl:5: error: non-void function "redclfunc" had no 'return' statement.

Part of the original fix delineated built-ins from user defined functions leading to more descriptive messages such where the larger error is probably a user trying to override a builtin which can be reported as such:

string concat (string a, string b) { return ""; }

test.osl:10: error: redefinition of builtin function: string concat (string, string).

---

The first two commits in this are fairly simple and I'm fine with loping off the last and opening a new PR devoted to redefinitions, or just merging/resolving the current conflicts here, but would like to avoid resolving conflicts only to repeat of the current situation of duplicated work.

[lgritz]

Oy, sorry, I guess I botched this, not realizing that there were two PRs simultaneously that were both addressing the double declaration problem.

Yours was probably better. I'm sorry to make more work for you, but perhaps the best way to proceed is to merge just the first two commits of this PR, and then submit a second PR with the third commit, plus fixes to whatever I broke.

[marsupial]

Ok, chopped of the conflict. First two should be easy enough to review.

[lgritz]

LGTM, merging.