[PATCH] D41279: [ThinLTO][C-API] Correct api comments

ben via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 18 09:44:01 PST 2017


bd1976llvm marked 5 inline comments as done.
bd1976llvm added inline comments.


================
Comment at: include/llvm-c/lto.h:786
+ * Sets the cache pruning interval (in seconds). A negative value sets the
+ * maximum possible pruning interval. An unspecified default value will be
+ * applied, and a value of 0 will be ignored.
----------------
kromanova wrote:
> kromanova wrote:
> > It looks like we are setting the default values in CachePruning.h, so the comment "An unspecified default value" could be substituted with "Default value of 1200 seconds (20 minutes) is applied".
> > 
> > Obsolete comments are confusing. When I called this API in Sony's proprietary linker, I relied on this particular comment and end up doing initialization myself in my code. BTW, Ben, since I noticed it, we need to remove this initialization code I talked about from our (Sony's) proprietary linker.  
> I would prefer mentioning one more time that negative values effectively disable pruning. Hopefully, after your change the umbrella comment above mentions it, but it will be nice to repeat it the comment directly related to this function.
I can't change the unspecified default comment as this is part of the api i.e. these defaults might change in the future.

I will mention the "effectively disabling" in another review as this patch has already been accepted.


================
Comment at: include/llvm-c/lto.h:787
+ * maximum possible pruning interval. An unspecified default value will be
+ * applied, and a value of 0 will be ignored.
  *
----------------
kromanova wrote:
> Also, a value of 0 is not ignored, but instead it forces the scan to occur. See comment in CachePruning.h 
> 
> However, please do not make this change now, leave it "as is". I will send a patch a little later to accept 0 interval in thinlto_codegen_set_cache_pruning_function (as well as in 2 other APIs for setting relative size and expiration).  I will do this change in the comment(s) at the same time as I do the functional change itself. 
> 
Great - those new cache policies will be useful to customers.


================
Comment at: include/llvm-c/lto.h:795
 /**
  * Sets the maximum cache size that can be persistent across build, in terms of
  * percentage of the available space on the the disk. Set to 100 to indicate
----------------
kromanova wrote:
> Please apply the same changes as you did for thinlto_codegen_set_cache_pruning_interval
> to the rest of thinLTO APIs responsible for pruning policy/incremental builds.
> Otherwise, the comments look inconsistent.
> 
> E.g. for thinlto_codegen_set_final_cache_size_relative_to_available_space,
> the following changes has to be made:
> 
> (1) across build -> across builds
> (2) An unspecified default value will be applied -> Default is 75% (see comment in CachePruning.h)
I can't change the unspecified default comment as this is part of the api i.e. these defaults might change in the future.

I will add the tidyups in another review as this patch has already been accepted.


================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:152
+  /// Cache policy: interval (seconds) between two prunes of the cache. A
+  /// negative value sets the maximum possible pruning interval. A value
+  /// of 0 will be ignored.
----------------
kromanova wrote:
> Please mention one more time here that negative values effectively disable pruning
I will mention the "effectively disabling" in another review as this patch has already been accepted.


https://reviews.llvm.org/D41279





More information about the llvm-commits mailing list