FIX clang now can compile GDIS!

[ovhpa]

Dear Prof. Rohl,

I find the problem which prevent clang compiler from compiling gui_edit.c as you report on #19 :D The solution was in gui_edit_dialog function:

p_i=0;
for(i=0;i<12;p_i++ , i++)
  g_signal_connect(GTK_OBJECT(CEDIT.transmat[i]), "changed",
  GTK_SIGNAL_FUNC(change_transmat), p_i);

with change_transmat defined as

void change_transmat(GtkWidget *w, gint i)
{
const gchar *text;
text = gtk_entry_get_text(GTK_ENTRY(transmat[i]));
if (i < 9)
  tmat[i] = str_to_float(text);
else
  tvec[i-9] = str_to_float(text);
}

in which clang failed to get the relation between p_i and i. Actually this relation was misleading to begin with. I now resolve on something less prone to errors, with calling from gui_edit_dialog function defined as:

for(i=0;i<12;i++)
  g_signal_connect(GTK_OBJECT(CEDIT.transmat[i]), "changed",
    GTK_SIGNAL_FUNC(change_transmat), NULL);

while change_transmat function now updates every fields on each call:

void change_transmat(GtkWidget *w)
{
gint i;
const gchar *text;
if(w==NULL) return;
  for(i=0;i<12;i++){
    text = gtk_entry_get_text(GTK_ENTRY(CEDIT.transmat[i]));
    if (i < 9)
      CEDIT.tmat[i] = str_to_float(text);
    else
      CEDIT.tvec[i-9] = str_to_float(text);
  }
}

While this is not equivalent I think we can afford to loose the little time needed to update 12 text fields instead of 1, and gain clang optimization.

Additionally: I also modified the gui_edit.c file to have its disseminated global variables inside the external global variable sysenv. I think this is more safe (and easy to follow) this way, but it is actually unrelated to this BUG.

Sincerely,

[arohl]

Have I missed something here? Why is p_i any different to i - they both start at zero and get incremented at the same time.

[ovhpa]

Sorry, I meant p_i in gui_edit_dialog and i in change_transmat. The problem is that p_i is defined as a pointer in the former function, but used as an integer in the latter. While this is OK with both gcc and clang (because pointer are integer most of the time) one optimization step of clang tried to unroll the for loop with the function and results in getting lost in the meaning of a pointer which address values are 1, 2, 3 ;)

[arohl]

That is ugly - you should never be able to set a pointer to address 0!

[ovhpa]

I agree, which is why I change to not passing/setting any pointer.

Another solution, more optimized, would be to create a global integer array containing {0,1, 2, 3, ..., 12} and to pass the pointer to each element to change_transmat (because g_signal_connect require that we pass a pointer). In that case clang would optimized properly a similar function:

void change_transmat(GtkWidget *w, gint *p_i)
{
const gchar *text;
gint i=*p_i;
text = gtk_entry_get_text(GTK_ENTRY(transmat[i]));
if (i < 9)
  tmat[i] = str_to_float(text);
else
  tvec[i-9] = str_to_float(text);
}

But (in my opinion) that would be a lot of change to make for a very small benefit ;)

[arohl]

Agreed

[ovhpa]

By the way, is the clang solution working for you?

[arohl]

Haven’t tried but another user contacted me and is happy with the fix