hadronized / glsl

GLSL parser for Rust
191 stars 27 forks source link

glsl-3.0: unresolved questions #96

Closed hadronized closed 4 years ago

hadronized commented 4 years ago

I’m about to release glsl-3.0 but there are still some things that make me uncomfortable. The current situation with the preprocessor is a bit uncertain, as we are typing it (e.g. here, here). In my opinion, we should only use String here as at the stage of preprocessing, GLSL types don’t really exist yet.

The main issue I have with that is linked to the actual usefulness of such a representation. Given the parsed AST, I wonder how easy it is to actually preprocess the AST:

fn preprocess(ast: TranslationUnit) -> Result<TranslationUnit, PreprocessorError>;

Maybe we can add that function to see how things are actually going on. In the meantime, it’s likely that I change those Expr to String.

hadronized commented 4 years ago

Ok, there’s something else I’m concerned with. The current status of #95 has a big problem to me: it inserts CPP directives in the AST, while preprocessing should be performed before parsing. The problem is that, after having parsed the input data, the generated AST (i.e. TranslationnUnit) contains unprocessed CPP directives. What it means is that some part of the code got parsed to basic types structures that are not correct.

Example:

#define FOO 3.14
float x = FOO;

The generated AST will have rhs an Expr::Var before preprocessing. However, replacing the value is going to need to change the type of the Expr to Expr::FloatConst or Expr.:DoubleConst. Consider:

#define f32 float
f32 x = 3.14;

Same problem here, but with a type. The type is going to resolved to a TypeName(Identifier::new("f32")) while it should be a Float type.

We can resolve that issue by implementing a CPP visitor, but that seems like an insane amount of work for such a task. Also, is it really in glsl’s scope?

hadronized commented 4 years ago

However, those CPP directives are a bit special because we might want to pass them down to an actual shader compiler. So I’ll just move types to String. If someone wants to actually preprocesss an item, I guess they will have to implement Visitor in a smart way.

hadronized commented 4 years ago

I’m adding support for macro with args, and it should be all good. The actual preprocessor, if someone wants it, will come later and can be a minor patch.

hadronized commented 4 years ago

Ok, two remaining problems (I’m creating issues for them):

hadronized commented 4 years ago

Everything got implemented, closing.