aaron.ballman added a subscriber: rsmith. aaron.ballman added a comment. Thank you for your patience with my delayed review, I've been at standards meetings for C and C++ and I'm a bit behind schedule.
> The AST dump tests were updated using the following script: One of the things @rsmith and I were chatting about was modifying this script so that you don't need to pass the location of Clang or duplicate RUN arguments to regenerate the tests. Richard wrote on the mailing list: >> I tried using this script to update a json dump test, and it seems to not >> really work very well: >> >> 1. it's really inconvenient to invoke; you need to pass in a --clang, copy >> some options out from two different places in the test file (from the RUN >> line and from a "--filters" line), and guess how to form the proper command >> line for the tool >> 2. it generates a file with the wrong name (adding a -json before the >> extension) >> 3. it doesn't strip out the CHECK lines from the previous run of the tool >> 4. it adds in a bunch of trailing whitespace, causing the file to not match >> the one in the repository >> >> Have you had a chance to look at making this more usable? I think the >> approach taken by utils/make-ast-dump-check.sh helps a lot: run the test in >> its normal configuration to generate the output, and then replace FileCheck >> with a tool that updates the CHECK lines instead of checking them. (That >> saves you needing to pass in --clang and --opts, at least, and makes it >> really easy to update a whole bunch of tests at once by running them all >> with lit.) It looks like you've hit some of the major points, and so I definitely think this is a big improvement over what we already have. Would you be interested in further modifications to address the concerns Richard brought up? (Do not feel obligated, because this is still a step in the right direction as-is.) ================ Comment at: clang/test/AST/gen_ast_dump_json_test.py:3 +from __future__ import print_function from collections import OrderedDict ---------------- Is this change needed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69564/new/ https://reviews.llvm.org/D69564 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
