yelite commented on code in PR #11971: URL: https://github.com/apache/tvm/pull/11971#discussion_r912011256
########## include/tvm/script/printer/doc_printer.h: ########## @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +#ifndef TVM_SCRIPT_PRINTER_DOC_PRINTER_H_ +#define TVM_SCRIPT_PRINTER_DOC_PRINTER_H_ + +#include <tvm/script/printer/doc.h> + +#include <memory> +#include <ostream> +#include <string> + +namespace tvm { +namespace script { +namespace printer { + +/*! + * \brief Configurable options for DocPrinter + * + * \sa DocPrinter + */ +struct DocPrinterOptions { + int indent_spaces = 4; +}; + +/*! Review Comment: Good question. Let me break this into smaller questions and answer them one by one: > Is there shared implementation between subclasses? The diagnostic message will be implemented in the DocPrinter. It's language agnostic and will be shared between subclasses. > Do we have to create a base class for shared code? No, the code related to diagnostic message could be encapsulated into a class that follows or have similar interface as `std::ostream`. > Then what's the benefit of having a base class? Because the Doc node types are not extensible, we know exactly what types of Doc the DocPrinter needs to handle. Coding this information as a base class makes things more discoverable. > Does it need to be in the public header? Not really, unless we want developer to implement DocPrinter in an external library. I don't think it's something we target for in the short term. I can remove DocPrinter from public headers. > For example, replace it with a method:... Good suggestion. In the TVMScript printer entry point, it only needs a function like `void PrintDocPython(Doc doc, DocPrinterOptions options, std::ostream& os);`. I will create this interface and hide necessary details. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
