Created attachment 105178 [details]
Vala source code (test case)

Created attachment 105179 [details]
Wrong generated C code

Confirming.

*** Bug 538166 has been marked as a duplicate of this bug. ***

This is currently blocking my porting of cheese. Do you have any pointers on how to fix this in vala. I can try to take a stab at this

(In reply to comment #5)
> This is currently blocking my porting of cheese. Do you have any pointers on
> how to fix this in vala. I can try to take a stab at this

You can work around this bug if you initialize the variabile in the class costructor (see attachment in comment #1)

For me that does not really work. I have a struct that I need to fill

What I want is to fill a const struct array that I use to lookup stuff

E.g

struct EffectData {
	Effect effect;
	string name;
	string filename;
	bool is_black;
} 

const EffectData[] EFFECT_DATA = {
	{Effect.NO_EFFECT, N_("No Effect"),
	 Config.PACKAGE_DATADIR + "/effects/identity.png", false}		
	};

*** Bug 549512 has been marked as a duplicate of this bug. ***

*** Bug 549513 has been marked as a duplicate of this bug. ***

Created attachment 117633 [details] [review]
Proposed fix

Created attachment 117634 [details]
A more complex test case

This is a more complex test case. Compile with:

valac -C --save-temps bug-516287-prop-init-consts.vala --vapidir . --pkg bug-516287-prop-init-consts

gcc -I . -o bug-516287-prop-init-consts bug-516287-prop-init-consts.c `pkg-config --cflags --libs glib-2.0 gobject-2.0`

Created attachment 117635 [details]
A more complex test case (Vapi file)

Created attachment 117636 [details]
A more complex test case (config.h)

Juerg, can you look at the patch in this bug. Thanks

Thanks for the patch. It looks mostly fine, however, I don't like the CCodeBinaryOperator.NONE part, as I've already mentioned on IRC some time ago, it's not a binary operator. I think it'd make sense to just combine the two CCodeConstant objects into a new CCodeConstant that contains `"foo" "bar"´. Can you update the patch?

Juerg we have already talked about combining the two costants in one, but as I already stated that woudn't solved the problem in comment #7.

So I now that you didn't like the NONE (was NOP) operator, but I don't see any other "simple" solution to the problem.

Having said that you know, may be I've missing something important...

I'm not talking about combining "foo" + "bar" into "foobar". What I'm proposing is to transform "foo" + "bar" in Vala to one CCodeConstant that contains `"foo" "bar"´. This will also work with DATADIR + "bar" in Vala to one CCodeConstant that contains `DATADIR "bar"´

Created attachment 119637 [details] [review]
Proposed fix #2

This patch try to follow what Jürg wrote on comment #17.

The only thing that I'm not sure about is this if:

if ((cleft is CCodeConstant || ((expr.parent_node is InitializerList || expr.parent_node is Field) && cleft is CCodeIdentifier))
&& (cright is CCodeConstant || ((expr.parent_node is InitializerList || expr.parent_node is Field) && cright is CCodeIdentifier))) { .....


especially the part expr.parent_node is InitializerList etc...

Anyway that code is required for initializers like DATADIR + "foo" for class fields and structures.

*** Bug 555047 has been marked as a duplicate of this bug. ***

*** Bug 555048 has been marked as a duplicate of this bug. ***

Thanks for the updated patch. The mentioned condition is certainly not correct in all cases as not all CCodeIdentifiers in an initializer have to be constants. However, it should fix the bug and I don't think that it causes regressions.

Created attachment 120294 [details] [review]
Cleaned bot not fixed patch

On a closer look, there are regressions, so I can't commit this patch yet. The following example code causes two memory leaks:

void main () {
	string foo = "foo";
	string bar = "bar";
	string result = foo + "hello" + bar;
}

The issue is probably that the semantic analyzer doesn't mark the result of the concatenation as owned.

Created attachment 120313 [details] [review]
New correct patch

This patch pass the make check and seems to fix this bug and the last regression. It's a little bit different from what we have said in IRC though

Please mind the comment around this line:

/* HACK: without this bad things happens: wrong c code, memory leaks, bug #538776...
How can we move this to SemanticAnalyzer too? */
expr.value_type.value_owned = false;

Created attachment 120315 [details]
Example of wrong C code with the last patch

This is the C code (wrong) that vala produces if the commented line in the last patch is omitted.

There are a lot of temp variables since vala thinks that all the string are owned even the constant ones.

Regard bug 538776, this code triggers the same bad behaviour, where local vars are added to a method (see test_fun_with_constants_test_method) and then used in the instance init one.

I attached this info to this bug because Jürg is not sure if there some correlation with the bug 538776 so he can check.

Thinking a bit more about the approach, it should be possible to avoid the hacks in the patch. To make it a bit easier, I'll commit a part of the patch now and will think about how to solve this properly, then.

2008-10-17  Jürg Billeter  <j@bitron.ch>

	* vala/valasemanticanalyzer.vala:
	* gobject/valaccodegenerator.vala:

	Move C-specific string concatenation from semantic analyzer to
	code generator, patch by Andrea Del Signore

2008-10-17  Jürg Billeter  <j@bitron.ch>

	* vala/valabinaryexpression.vala:
	* vala/valaexpression.vala:
	* vala/valaliteral.vala:
	* vala/valamemberaccess.vala:
	* vala/valasemanticanalyzer.vala:
	* gobject/valaccodegenerator.vala:

	Treat the result of two concatenated string constants as constant,
	fixes bug 516287

Fixed in r1848.