r196407 - Enea Zaffanella's fix for the PPCallbacks Elif callback, with a slight re-org, and an update of the new PPCallbacks test (soon to be moved to clang from extra), rather the unittest.

[Thompson, John]

Perhaps this follows at least one convention I see more closely for enum value naming, since the coding standard doesn't spell it out: 

enum ConditionValueKind { CVK_True, CVK_False, CVK_NotEvaluated };

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Thompson, John
Sent: Thursday, December 05, 2013 1:07 PM
To: Enea Zaffanella
Cc: cfe-commits at cs.uiuc.edu
Subject: RE: r196407 - Enea Zaffanella's fix for the PPCallbacks Elif callback, with a slight re-org, and an update of the new PPCallbacks test (soon to be moved to clang from extra), rather the unittest.

> I was wondering if a further boolean parameter to the callback (IsConditionEvaluated or similar) would be the simplest approach to let the clients know whether or not they should trust the value passed in parameter ConditionValue.

Another option would be to instead use an enum to represent the three states, i.e.  enum ConditionValueKind { CVTrue, CVFalse, CVNotEvaluated };

Does anyone have an opinion on this?

Otherwise I'll go ahead and change it to use the enum.

-John

-----Original Message-----
From: Enea Zaffanella [mailto:zaffanella at cs.unipr.it] 
Sent: Thursday, December 05, 2013 12:06 AM
To: Thompson, John
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: r196407 - Enea Zaffanella's fix for the PPCallbacks Elif callback, with a slight re-org, and an update of the new PPCallbacks test (soon to be moved to clang from extra), rather the unittest.


On 12/04/2013 09:35 PM, Thompson, John wrote:
> Hi Enea,
>
> Thank you for fixing the Elif callback call in PPCallbacks.  This checkin (r196407) contains your fix (with a minor rearrangement), except that rather than use your change to the PPCallbacks unit test, I update the pp-trace test to test this, which is in the related clang-tools-extra checkin.  I will soon be moving this tool and test to clang, unless someone objects.
>
> -John

Thank you.

If you are currently looking at PPCallbacks, I would like to point out the following:

   /// \brief Hook called whenever an \#elif is seen.
   /// \param Loc the source location of the directive.
   /// \param ConditionRange The SourceRange of the expression being tested.
   /// \param ConditionValue The evaluated value of the condition.
   /// \param IfLoc the source location of the \#if/\#ifdef/\#ifndef directive.
   // FIXME: better to pass in a list (or tree!) of Tokens.
   virtual void Elif(SourceLocation Loc, SourceRange ConditionRange,
                     bool ConditionValue, SourceLocation IfLoc) {
   }

Here it is said that ConditionValue holds the evaluated value of the condition.
However, this callback is also invoked when we already have taken a previous (#if or #elif) branch and in this case the condition is not evaluated at all.
Hence, the boolean value passed to the callback is just meaningless.
This happens in method:

     void Preprocessor::HandleElifDirective(Token &ElifToken)

I was wondering if a further boolean parameter to the callback (IsConditionEvaluated or similar) would be the simplest approach to let the clients know whether or not they should trust the value passed in parameter ConditionValue.

Enea.




_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits