On Fri, Aug 29, 2014 at 1:06 AM, Alexander Kornienko <[email protected]> wrote:
> On Fri, Aug 29, 2014 at 6:55 AM, David Blaikie <[email protected]> wrote: > >> >> On Thu, Aug 28, 2014 at 8:03 PM, Sean Silva <[email protected]> >> wrote: >> >>> >>> On Wed, Aug 27, 2014 at 4:28 PM, Alexander Kornienko <[email protected]> >>> wrote: >>> >>>> On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <[email protected]> >>>> wrote: >>>> >>>>> On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko < >>>>> [email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
