hadronized / glsl

GLSL parser for Rust
191 stars 27 forks source link

shared multidim arrays parsed incorrectly #130

Closed vron closed 4 years ago

vron commented 4 years ago

The latest version (4.1.1) incorrectly parses shared variables which are multidimensional arrays:

#version 450
shared uint mlt2[1][2];  // This row is incorrectly parsed as syntax::Declaration::Global instead of syntax::Declaration::InitDeclaratorList

To reproduce run:

extern crate glsl;
use glsl::parser::Parse;
use glsl::syntax;
use glsl::transpiler::glsl::show_translation_unit;

fn main() {
    let contents = String::from("
#version 450
shared uint mlt2[1][2];  // This row is incorrectly parsed as syntax::Declaration::Global instead of syntax::Declaration::InitDeclaratorList
´");

    let tu = syntax::TranslationUnit::parse(contents).unwrap();
    let mut output = String::new();
    show_translation_unit(&mut output, &tu);
    println!("{}", output);
}

And note that the output is:

#version 450
shared;

Since the parser has parsed the line "shared uint mlt2[1][2];" into a syntax::Declaration::Global instead of syntax::Declaration::InitDeclaratorList the syntax tree does not retain enough information for the transpiler to print it?

vron commented 4 years ago

Hmm, when trying to fix this it seems deeper than I though, are multidimensional arrays not supported at all? (they are in glsl since 4.3 to my understanding)

E.g. there is the definition:

pub enum ArraySpecifier {
  Unsized,
  ExplicitlySized(Box<Expr>),
}

Should the explicitlySized case be a Vec of expressions instead? I was planning to implement this but am hesitant to start changing to much (since to my understanding this public API would need to change) before discussion.

hadronized commented 4 years ago

Yeah, I basically made a test with glsl-tree (the PR is on its way to merge to master, it’s a simple tool to debug the AST). Yes, indeed, they don’t seem supported right now. I’m having a look at the GLSL450 spec to see how I missed that. :D

hadronized commented 4 years ago

img_072720_142052

That’s the definition… it looks like it supports 2D? yeah it supports them all.

vron commented 4 years ago

👍 - great, would be happy if they were supported, using your crate (been great so far) to write a transpiler for compute shaders, and need the Md-case

vron commented 4 years ago

Yes, that indeed seems as if it is in the spec too (matches what google tells me :-P )

hadronized commented 4 years ago

Great. So that should be easy to support. I can do it if you don’t have the time to work on it. :)

vron commented 4 years ago

That would be ideal (new to rust, so anything i did would probably need major review by you anyway)

hadronized commented 4 years ago

Alright!

vron commented 4 years ago

FYI: tried your patch on my test case and it is all 👍