Bugfix var local name by yannilefki · Pull Request #44 · charguer/optitrust

yannilefki · 2026-07-06T13:19:15Z

Fixed the span bug with Variable.local_name.
For that I :

Created a new file "seq_helper.ml" to solve module dependencies
Added a handling of the Dir_span case in two functions of "path.ml"

p.s. : the modification of ShowAt is to solve another documented dependency issue, which has to link to this branch.

Bastacyclop · 2026-07-06T14:52:18Z

it's good practice to extend the local name unit test with a few cases that test the span feature.

Bastacyclop · 2026-07-06T15:09:59Z

This is an interesting solution, so basically if you are targeting a span, you create a sub-sequence on-the-fly so that transformations may treat is as a regular sequence node without explicit treatment. Note that this is not how we programmed similar transformations, see assert_instr_effects_shadowed Accesses_basic.transform_on, Function_core.uninline_on`, ... (grep 'update_span_helper'). The question would be : what are the pros and cons to having transformations explicitly manipulate spans vs the target system manipulating spans transparently.

yannilefki · 2026-07-08T09:41:49Z

+        end else begin
+          let (span_instrs, _instrs_after) = Mlist.split ~left_bias:false stop instrs in
+          let (_instrs_before, span_instrs) = Mlist.split ~left_bias:true start span_instrs in
+          let t_seq = trm_seq_nobrace ?result span_instrs in


In the end I did not modify to trm_seq_nobrace.
The No_brace modules asks for any nobrace sequence to be wrapped with a Nobrace_transfo.remove_after, which would be difficult in this situation, since this function is called for its output.
To be more precise, taking the unittest I wrote as an example, I would get an execution failure from Nobrace.current, because there is no live 'id' to use as a Nobrace annotation.
For me to be able to wrap with Nobrace_transfo.remove_after, I would need a 'unit -> unit' function, this means that I would need to encapsulate many function calls inside, which I believe is not a good idea.

then you should try a unit test where you call the transformations on a subsequence containing an allocation, which probably won't work:

/* @target-begin */ int a = 0; b++; /* @target-end */ a++;

in this case it seems like creating sub-sequences on-the-fly during target resolution, although tempting because unifying APIs, might not be the way to go.

Bastacyclop · 2026-07-08T10:00:36Z

-        Seq_helper.update_span_helper span t (fun tl -> [Trm (aux (trm_seq tl))])
+        update_span_helper span t (fun tl -> [Trm (aux (trm_seq tl))])


One could also consider keeping it qualified but short as in Seq_helper.update_span. In general, I think our current codebase is doing too much module opening and namespace flattening.

I don't think I understand your point? Do you mean by that that function names are too long? You are right that we could for example change from Seq_helper.update_span_helper to Seq_helper.update_span, as it would not change the meaning of the function's name.

I mean that we use ad-hoc prefixes like trm_add instead of nice modules like Trm.add that act as prefixes just as well.

Bastacyclop · 2026-07-08T10:02:37Z

-  if span.start >= span.stop then begin
-    t_seq


I think part of the point for this test was to speed-up the span.start = span.stop no-op case.

Bastacyclop · 2026-07-08T10:03:43Z

  !! Variable.local_name ~var:"a" ~local_var:"x"
    [cFunBody "ko_scope"; cLabel "l"];
+
+  !! Variable.local_name ~var:"a" ~local_var:"x" [cFunBody "ok3"; tSpanSeq [cForBody "i"]];


try doing more complex spans that don't start/end exactly on a sequence, and that contain allocations whose scope matters.

…xps. Meaning that any target with a regular expression (e.g. `sInstr`) was failing.

…rks.

2. Removed the "failure_expected" tests, since they are deprecated with models. 3. Added comments on flags for some cleaning later (wip).

The actual behavior is correct (or at least as expected), but the test being syntaxic means that any renaming is seen as a failure from the builder.

yannilefki · 2026-07-16T14:45:37Z

+(* TODO Yanni : reevaluate *)
 (** [preserve_specs_only]: allow code transformation that preserve the specification without necessarily preserving the semantics
    TODO: update code which was also using check_validity for this purpose *)
 let preserve_specs_only = ref false


This is equivalent to [Annotated] mode.

yannilefki · 2026-07-16T14:50:52Z


+(* TODO Yanni : reevaluate *)
 (** [check_validity]: perform validation of transformations *)
 let check_validity = ref false


This is now equivalent to mode [AnnotatedAndVerified], and is not expected to be asked by transformations anymore.

Bastacyclop · 2026-07-21T14:19:10Z

-  !! Variable.local_name ~var:"a" ~local_var:"x"
-    [cFunBody "ko_scope"; cLabel "l"];


Bastacyclop · 2026-07-21T14:23:38Z

  !! Variable.local_name ~var:"a" ~local_var:"x" [cFunBody "ok1"; cFor "i"];
  !! Variable.local_name ~var:"a" ~local_var:"x" [cFunBody "ok2"; cLabel "l"];
-
+(*


TODO: use and improve Trace.resource_error_expected

yannilefki added 2 commits July 6, 2026 11:46

working on dependencies

50bbebe

solved the span issue with Variable.local_name

7218943

Bastacyclop reviewed Jul 6, 2026

View reviewed changes

Comment thread case_studies/dot_product/dot.ml Outdated

Bastacyclop reviewed Jul 6, 2026

View reviewed changes

Comment thread lib/framework/target/path.ml Outdated

Bastacyclop reviewed Jul 6, 2026

View reviewed changes

Comment thread lib/framework/target/path.ml

yannilefki added 3 commits July 7, 2026 12:54

command for dependency debugging

2a5405b

answered commits and resolved small mistakes

caaebf0

more readable viewdoc documentation for dark theme users

37c729e

yannilefki commented Jul 8, 2026

View reviewed changes

writing unit tests and commenting

cbcd027

Bastacyclop reviewed Jul 8, 2026

View reviewed changes

yannilefki added 5 commits July 9, 2026 18:30

bugfix : the call to resolve_target in find_var was singling out rege…

d395d96

…xps. Meaning that any target with a regular expression (e.g. `sInstr`) was failing.

unit tests. wip : adding it to batch.ml after understanding how it wo…

73b0843

…rks.

small debug flags

4d20b91

1. Finished unit tests and polishing comments.

b50ae1f

2. Removed the "failure_expected" tests, since they are deprecated with models. 3. Added comments on flags for some cleaning later (wip).

I commented a test that raised a failure.

73f90ca

The actual behavior is correct (or at least as expected), but the test being syntaxic means that any renaming is seen as a failure from the builder.

yannilefki commented Jul 16, 2026

View reviewed changes

Bastacyclop reviewed Jul 21, 2026

View reviewed changes

Comment thread lib/framework/target/path.ml

Bastacyclop reviewed Jul 21, 2026

View reviewed changes

Comment thread lib/transfo/variable_basic.ml Outdated

PR cleaning

52d36de

yannilefki merged commit 8078712 into main Jul 21, 2026
1 check passed

		Seq_helper.update_span_helper span t (fun tl -> [Trm (aux (trm_seq tl))])
		update_span_helper span t (fun tl -> [Trm (aux (trm_seq tl))])

		!! Variable.local_name ~var:"a" ~local_var:"x"
		[cFunBody "ko_scope"; cLabel "l"];

Conversation

yannilefki commented Jul 6, 2026

Uh oh!

Uh oh!

Bastacyclop commented Jul 6, 2026

Uh oh!

Uh oh!

Uh oh!

Bastacyclop commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants