> I'm sorry recent weeks I've got busy
No problem. I am sorry to reply this late to your message as well.
> Later is the response from previous mail.
Please do not take this bad, but it will really ease communication (and
the review) if, when you reply to this message, you write your answers
*below* the parts of you are replying to, and remove the parts (of my
message) you are not replying to. I believe this is a good habit to
have at least when you are interacting with the GCC mailing lists.
Now here are my comments on your patches.
> -------------------------
> macro_end_expand can't be moved to _cpp_pop_context. My plugin shows later
> results when `Gs callee _cpp_pop_context'.
> GS multiple definition:
> 1)libcpp/directives.c 8130 skip_rest_of_line DEF_FUNC
> 2)libcpp/macro.c 67161 expand_arg DEF_FUNC
> 3)libcpp/macro.c 72900 cpp_get_token_1 DEF_FUNC
> 4)libcpp/traditional.c 10560 _cpp_scan_out_logical_line DEF_FUNC
I still think that the macro_end_expand can and should be moved to
_cpp_pop_context, even though that function is called from multiple
places. That is, in the 'if' block:
if (context->c.macro)
{
you just have to write something like:
if (in_macro_expansion_p)
/* We are at the end of the expansion of a macro. */
macro_end_expand (...);
This is what I was trying to tell you in my previous message, when I was
saying:
> _cpp_pop_context is really the function that marks the end of a
> given macro expansion, especially when the predicate
> in_macro_expansion_p (introduced recently in trunk for gcc 4.8)
> returns true.
Am I missing something?
> So macro_start_expand can't be moved to enter_macro_context since the pair
> should be matched in the same function cpp_get_token_1.
In the light of what I said above, I think macro_start_expand should
be moved to enter_macro_context, if you agree with my previous
statements.
> 1) macro_start_expand and macro_end_arg are used to tag all macro
tokens.
I think there are some concepts that need to be a bit more clearly
stated here, otherwise we'll be talking past each other.
You are basically making the pre-processor emit events at some useful
points of its operations. And then, your client code (the code of
your plugin) reacts whenever these useful events are emitted. It
reacts to these events to accomplish some useful tasks.
The tasks your wants to accomplish here, somehow, is to mark the tokens
that results from the expansion of a macro.
Right?
I think it will make the code more maintainable to have the events be
emitted at points that are "well defined". And this is where I am
having issues with your patch.
So, it seems clear to me that macro_start_expand is an event that is
emitted at the beginning of the expansion of a macro.
But the "meaning" of the macro_end_arg event is really not clear to
me. From your code, it looks like it's a hack that lets you detect if
the macro_start_expand event really was emitted in cases where we are
sure that the macro is going to be expanded.
Let me explain a bit more.
When a macro M is encountered, enter_macro_context triggers its
expansion; but sometimes it does not. And what I understand is that, in
an ideal world, you want to call macro_start_expand only in cases where
enter_macro_context actually triggers the expansion. That way, your
plugin can use the tokens of the replacement-list of M (passed to
macro_start_expand) as it sees fit.
But in this patch, you are going a hackish route by unconditionally
calling macro_start_expand whenever M is encountered (right after
enter_macro_context is called), and if enter_macro_context does *not*
trigger the expansion of M, you tell your plugin to "back out", by
calling macro_end_arg with its 'cancel' parameter set to a 'true'
boolean. Otherwise, if enter_macro_context happens to really have
expanded M, you are calling macro_end_arg with its 'cancel' parameter
set to a 'false' value, effectively telling your plugin "OK, M was
really expanded".
And what I was saying in my previous email is that you could arrange
to emit the macro_start_expand event *only* when you are sure that
enter_macro_context is really going to expand M.
That is why I was saying in my previous message:
> IMHO, it's more maintainable to put the call to
> pfile->cb.macro_start_expand inside enter_macro_context, as that
> is the function that really triggers the macro expansion.
And then, I told you where to put that call in enter_macro_context so
that you are sure to emit the event only when you are sure
enter_macro_context is going to expand M (and I am repeating it here
again):
> Maybe if you put the call to macro_start_expand in this
> enter_macro_context function only after we are sure we are going
> to actually proceed with the expansion ....
>
>> return 0;
>> }
>
> ... here, then you wouldn't need the macro_end_arg at
> all. Would that make sense?
> 2) macro_end_arg and macro_end_expand are used to tag all macro-replacement
> tokens.
To be more precise I'd say that macro_end_expand is an event which
meaning is "This is the end of the expansion of the macro that was
signaled by a previous invocation of macro_start_expand". And your
plugin reacts to that event by tagging the tokens of the
replacement-list of the macro.
> So macro_start_expand can't be moved to enter_macro_context when I'm
> sure a macro expansion has occured.
I am not sure to understand what you mean here. If you call
macro_start_expand at the *beginning* of enter_macro_context as I
explained above, then the macro expansion has not *yet* occurred. But
you are sure that it *will* occur.
> To make potential user free resource, macro_end_expand is called
> even macro is canceled. That's why macro_end_expand is called twice
> in cpp_get_token_1.
If you call macro_start_expand only when you are sure that the macro
is going to be expanded, I believe you don't need to call
macro_end_expand twice.
> If gcc can't accept cpp_token::file_offset field, I can change my plugin
> stores token linenum other than token file_offset to database, final user
> should be tolerate with it.
You still don't reply to the key questions that I have asked you in my
previous email:
> So why is this file_offset member needed?
>
> From the rest of the patch, it looks like you are using it to
> store the offset of the token, from the beginning of its
> line. What is the difference between that, and the column
> number of the token, as you could get by calling
> linemap_expand_location on the src_loc of the token, effectively
> doing away with this new file_offset member? Note that the
> linemap_expand_location call was introduced in 4.7.
Please reply to these questions so that I understand why you need this
field at all, rather than getting it from the linemap infrastructure
by calling, for instance, linemap_expand_location.
When these questions are sorted, I think we can go further with the rest
of the review.
> By the way, have you tried my plugin in previous mail?
Not yet. For now, I am reading the patch. :-)
Thank you for your time and efforts.
--
Dodji
