Elnatan,
Thank you very much for your patch.
On Thu, Sep 16, 2010 at 06:33:34PM -0400, Elnatan Reisner wrote:
> The attached patch addresses those three items, and it also
> introduces a command-line option to set the useLogicalOperators
> flag.
I'm willing to commit those, but check.ml should be fixed first:
$ cd test && make testrun/logical3 EXTRAARGS="--useLogicalOperators"
yields:
First CIL check
logical3.c:4: Warning: CIL invariant broken: Type mismatch:
long long
and int
Bug: CIL's internal data structures are inconsistent (see the warnings
above). This may be a bug in CIL.
Moreover, it would be nice if logical3.c could raise an error (with
macros in testharness.h). I am not sure how logical2.c could be
automatically tested so I'll leave it as is.
Elnatan, could you have a look? Things are ready in my local
repository, just waiting for proper tests.
> First, CIL already had a check to see if the first operand of && and
> || is constant, and it does a form of constant-folding in such
> cases. I added a similar check for the second operand, when it does
> not have side effects. One result of this, however, is that
> something like '*p && 0' will become simply '0', and if p was null,
> this means that the transformed program does not crash, whereas the
> original would have. I would argue that this an acceptable
> transformation, but I'm curious to hear if others disagree.
I agree that CIL might perform this optimisation, given the behaviour is
undefined, but I am reluctant to perform such changes without a way for
the programmer to disable it. What about doing it only if
lowerConstants is enabled (which is the case by default)? Or adding a
new option in the lowering subgroup to control it?
The following blog post is also worth reading:
http://blog.regehr.org/archives/232
In particular the update about CompCert (which uses CIL as a frontend).
> Second, this patch also (re-)introduces a Question variant for the
> Cil.exp type, representing a side-effect free use of the conditional
> operator '?:'.
Sorry, but I do not want to commit this one, mainly because it changes
the CIL's AST, which means every CIL program should be updated to take
it into account.
> I addressed all of the compiler warnings about non- exhaustive
> pattern-matchings, but I'm not entirely sure I handled all of them
> properly. I'm also not sure the compiler warnings tell me everywhere
> that needs to change. (This patch only changes 8 files.) If someone
> who knows more than I about those pieces wants to double-check, that
> would be great. Even if the CIL owners don't want to reintroduce
> Question, I'd like to know if anyone notices anything obviously wrong
> about my modifications, because I think I want to use this myself.
Modifying files based on compiler warnings has always been very
effective for me (when extending CIL with more constructs). Your patch
seems correct, but I only looked at it briefly.
Best regards,
--
Gabriel Kerneis
------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
CIL-users mailing list
CIL-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cil-users
