<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 29, 2014 at 1:06 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Fri, Aug 29, 2014 at 6:55 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><div>On Thu, Aug 28, 2014 at 8:03 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><div>On Wed, Aug 27, 2014 at 4:28 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: alexfh<br>
Date: Wed Aug 27 16:36:39 2014<br>
New Revision: 216620<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216620&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216620&view=rev</a><br>
Log:<br>
Query CompilationDatabase right before running each compilation.<br>
<br>
Summary:<br>
Query CompilationDatabase right before running each compilation. This allows<br>
supporting compilation databases that change external state required for<br>
successful compilation.<br></blockquote><div><br></div></div><div>Could you give an example of this? It's not clear to me what this means.</div></div></div></div></blockquote><div><br></div></div><div>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.<br>
</div></div></div></div></blockquote><div><br></div></div><div>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?)</div>
</div></div></div></blockquote><div><br></div></div><div>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.<br>
</div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Also, what is there to stop individual compilations of the same file from having incompatible state?<br></div></div></div></div></blockquote><div><br></div></div><div>Yeah, it's certainly worth documenting what the contract is here (& maybe considering other APIs).<br>
<br>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).</div>
</div></div></div></blockquote><div><br></div><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>-- Sean Silva</div></div><br></div></div>