Closed tillspeicher closed 7 years ago
I addressed all the issues, it should be ready to merge now.
I've also polished a bit and fixed some potential problems. Can you plz have a look at my chagnes that I didn't break anything?
Note that I have removed the static method get_flags
. Simply use, for instance: HasSideEffects | IsAlignStack
. Maybe, you want to reuse in impala the same enum. So you don't have to convert stuff.
I'm just updating Impala based on the changes you made.
Using the Flags
enum there would be nice, but then we need to include primop.h
in ast.h
which I guess we don't want.
In this case we should move the enum somewhere else and include that.
Is enums.h
the right place for that?
Simply include primop.h
. That's fine.
Hi,
the patch looks nice :)
Some minor technical remarks:
Asm
would be the perfect name for this node, but sinceasm
is already a C++ keyword I suggestAssembly
- this is less cryptic thaninl_asm
and then we can useassembly
as variable nameTypes
instead ofstd::vector<const Type*>&
Defs
instead ofstd::vector<const Def*>&
ArrayRef<std::string>
instead ofstd::vector<std::string>
in the constructor andArray<std::string>
instead ofstd::string<std::string>
as member.World
wheremem
can be specified as extra input item.const Location& loc
should be the last argument.enum
with all flags instead of passing a ton of boolean flags - it's also easier to add further flags if this is desired.To sum it up, the constructor should look like this:
Same story for the factory method + a second method which looks like this: