r216620 - Query CompilationDatabase right before running each compilation.

[Sean Silva]

On Fri, Aug 29, 2014 at 1:06 AM, Alexander Kornienko <alexfh at google.com>
wrote:

> On Fri, Aug 29, 2014 at 6:55 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>> On Thu, Aug 28, 2014 at 8:03 PM, Sean Silva <chisophugis at gmail.com>
>> wrote:
>>
>>>
>>> On Wed, Aug 27, 2014 at 4:28 PM, Alexander Kornienko <alexfh at google.com>
>>> wrote:
>>>
>>>> On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <chisophugis at gmail.com>
>>>> wrote:
>>>>
>>>>> On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko <
>>>>> alexfh at google.com> wrote:
>>>>>
>>>>>> Author: alexfh
>>>>>> Date: Wed Aug 27 16:36:39 2014
>>>>>> New Revision: 216620
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=216620&view=rev
>>>>>> Log:
>>>>>> Query CompilationDatabase right before running each compilation.
>>>>>>
>>>>>> Summary:
>>>>>> Query CompilationDatabase right before running each compilation. This
>>>>>> allows
>>>>>> supporting compilation databases that change external state required
>>>>>> for
>>>>>> successful compilation.
>>>>>>
>>>>>
>>>>> Could you give an example of this? It's not clear to me what this
>>>>> means.
>>>>>
>>>>
>>>> This means, that in some implementations the call to
>>>> Compilations.getCompileCommands(File) may make changes in the file system
>>>> to allow compilation of the File (e.g. generate headers from .td files).
>>>> State of the file system required for compiling one file may be
>>>> incompatible with the state required for compiling another file, so we
>>>> actually need to run the tool on the file right after we call
>>>> getCompileCommands for this file.
>>>>
>>>
>>> This is really unusual. I wouldn't expect a call getCompileCommands to
>>> be mucking about on a filesystem. Especially since it is marked const.
>>> Maybe split out a specific method for preparing the filesystem for
>>> compilation of the file? IIRC, in C++11 it's really a very bad idea to have
>>> a const method that is not safe to call in parallel (David, do you remember
>>> what the rule is for this?)
>>>
>>
>> I don't believe C++11 actually makes this any worse - while C++11 does
>> have defined threading semantics, it doesn't actually require that types
>> are thread safe. I know Herb or others have certainly talked about
>> definitions of "thread compatibility" that are related to const-ness, but I
>> don't think any of that is part of the standard. I could be wrong, though.
>>
>>
>>> Also, what is there to stop individual compilations of the same file
>>> from having incompatible state?
>>>
>>
>> Yeah, it's certainly worth documenting what the contract is here (& maybe
>> considering other APIs).
>>
>> Probably wouldn't be too bad to make it non-const and/or have a
>> "prep/end" function or somesuch (make return a move-only token that
>> represents the valid state - when that's destroyed the state is no longer
>> required - but perhaps that's just overengineering).
>>
>
>
> We could add pre-post methods or do something else, but it would serve a
> purpose of one edge case, which we want to avoid eventually anyway. There
> was no particular reason to query compilation database in advance, so this
> change add support of this edge case by flipping to an otherwise equal
> alternative. I can document this in a comment, but I don't think we need to
> invest any more time in developing this workaround further.
>

Edge cases are precisely the most important things to document *and test*.
This patch neither documents nor tests this edge case, and it should. If
it's something that we want to eventually remove, then also add a `FIXME`
or `HACK` type comment to make it clear.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140829/45975ef8/attachment.html>